Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

Sign up for GitHub

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

NON-NLS more difficult example #46

Open
wants to merge 1 commit into
base: develop
Choose a base branch
from
Open

NON-NLS more difficult example #46

Show file tree
Hide file tree
Changes from all commits
Commits
Show all changes
1 commit
Select commit
File filter

Filter by extension

Filter by extension
Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
2 changes: 2 additions & 0 deletions palantir-java-format/src/test/resources/com/palantir/javaformat/java/testdata/NON-NLS.input
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -17,4 +17,6 @@ class MultilineStringConstant {
"field_3," +
"field_4";

private static final String SINGLE_LINE_NON_NLS = "field_0," + "field_1," + "field_2," + "field_3," + "field_4"; //$NON-NLS-1$ //$NON-NLS-2$ //$NON-NLS-3$ //$NON-NLS-4$ //$NON-NLS-5$
Copy link
Contributor

Choose a reason for hiding this comment

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

Is this for real??
I'd like to have a look at a comprehensive set of rules showing all the ways that these comments work.

  • do we need an extra // for each $NON-NLS instance?
  • does each comment map to a string literal, or could they be mapped to string constants / variables as well?
  • do we only need to handle string concatenations or other AST shapes too?

Copy link
Contributor Author

Choose a reason for hiding this comment

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

  1. Yep, that's correct
  2. Each comment is associated with a string literal, these are string constants in source
  3. This applies beyond concatenation, for example return getFoo("a", "b") //$NON-NLS-1$ //$NON-NLS-2$ could be rewritten to
return getFoo(
    "a", //$NON-NLS-1$
    // note that the index is updated from 2 to 1 after a line split
    "b") //$NON-NLS-1$

Copy link
Contributor

Choose a reason for hiding this comment

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

Ok. I think for the sake of simplicity, we're going to have to force the comments to come right after each string literal + random other language tokens that are attached to it unbreakably (like ,;)), and always give them a $NON-NLS-1$ tag as a result. Otherwise we can't be sure, at the level where we process this, that the lines won't be broken later.

How does that sound?

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Sounds reasonable to me :-)


}
6 changes: 6 additions & 0 deletions palantir-java-format/src/test/resources/com/palantir/javaformat/java/testdata/NON-NLS.output
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -15,4 +15,10 @@ class MultilineStringConstant {

private static final String MULTIPLE_LINE_NO_COMMENT =
"field_0," + "field_1," + "field_2," + "field_3," + "field_4";

private static final String SINGLE_LINE_NON_NLS = "field_0,"
+ "field_1,"
+ "field_2,"
+ "field_3,"
+ "field_4"; //$NON-NLS-1$ //$NON-NLS-2$ //$NON-NLS-3$ //$NON-NLS-4$ //$NON-NLS-5$
}