Skip to content

Conversation

lslonina
Copy link

@lslonina lslonina commented Jun 6, 2025

  • Refactor creation to avoid redundant canonicalization calls
  • Introduced internal factory methods for clearer construction paths

Overview


I hereby agree to the terms of the JUnit Contributor License Agreement.


Definition of Done

mpkorstanje

This comment was marked as duplicate.

@mpkorstanje
Copy link
Contributor

mpkorstanje commented Jun 6, 2025

I'm not sure why my review got posted twice. I've hidden one, but now they're both hidden. 😕

@ALUMINIS650

This comment was marked as outdated.

@lslonina
Copy link
Author

lslonina commented Jun 6, 2025

@mpkorstanje Thanks for the feedback — the code can definitely be simplified, especially with the instance method approach. Let’s see how the discussion on this topic evolves; I’ll keep this marked as a draft for now.

@lslonina lslonina marked this pull request as draft June 6, 2025 10:46
@marcphilipp marcphilipp linked an issue Jun 6, 2025 that may be closed by this pull request
*/
@API(status = EXPERIMENTAL, since = "6.0")
public FileSource withPosition(FilePosition filePosition) {
Preconditions.notNull(filePosition, "position must not be null");
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Suggested change
Preconditions.notNull(filePosition, "position must not be null");
Preconditions.notNull(filePosition, "filePosition must not be null");

@mpkorstanje
Copy link
Contributor

@mpkorstanje Thanks for the feedback — the code can definitely be simplified, especially with the instance method approach. Let’s see how the discussion on this topic evolves; I’ll keep this marked as a draft for now.

@lslonina the issue has moved off team discussion. Do you have time available to patch up your PR or would you prefer to hand it over?

@lslonina
Copy link
Author

Thanks for picking this up. I’ve added @API(status = EXPERIMENTAL, since = "6.0"), but if this is a backport candidate, should we change it to since = "5.13.5" instead?

@API(status = EXPERIMENTAL, since = "6.0")
public FileSource withPosition(FilePosition filePosition) {
Preconditions.notNull(filePosition, "filePosition must not be null");
return new FileSource(this.file, filePosition);
Copy link
Member

@marcphilipp marcphilipp Sep 12, 2025

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Please revert the changes to the constructors and factory methods above and introduce a copy constructor as proposed in #4630 (comment) instead.

Suggested change
return new FileSource(this.file, filePosition);
return new FileSource(this, filePosition);

@marcphilipp
Copy link
Member

Thanks for picking this up. I’ve added @API(status = EXPERIMENTAL, since = "6.0"), but if this is a backport candidate, should we change it to since = "5.13.5" instead?

Good question! Please keep using 6.0. We'll adjust it when backporting.

@marcphilipp
Copy link
Member

@lslonina Are you going to finish this PR or is @mpkorstanje taking over?

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Projects
None yet
Development

Successfully merging this pull request may close these issues.

Introduce withPosition() "wither" method in FileSource
5 participants