Community
Participate
Working Groups
The following code extract raises a 'Non-externalized string' warning: String nonNLS = "non NLS"; // $NON-NLS-1$ whereas this one does not (as expected): String nonNLS = "non NLS"; //$NON-NLS-1$ I would suggest that the first case be recognized as equivalent to the second one. To enlarge the case, the following may also be considered: OK: String nonNLS = "non NLS" + "tail"; //$NON-NLS-1$ //$NON-NLS-2$ currently KO: String nonNLS = "non NLS" + "tail"; //$NON-NLS-1$ $NON-NLS-2$ String nonNLS = "non NLS" + "tail"; //any_word$NON-NLS-1$ //$NON-NLS-2$
I disagree with this. NLS markers should remain simply and we should not tolerate whitespaces in them in order to improve detection.
I would contend that simplicity does not have the same meaning here for the implementor and for the user...
Has the view of the JDT team changed on this over the past year? Is this something you would consider fixing/enhancing?
*** Bug 215466 has been marked as a duplicate of this bug. ***
I am not sure how much overhead this would create, considering we are already able to detect 2nd tag in: //$NON-NLS-1$ //$NON-NLS-2$ (i.e. we still look forward...)
Right we search for //$NON-NLS-, we could replace this and search for $NON-NLS-. In term of speed, this should be evaluated. Howewer we would need to adjust all the tooling for removing and adding new tags. These tags are not meant to be manually edited, quickfixes are available to add or remove them.
Martin - can you comment on quickfix impact if we did recognize more flavours of NON-NLS tags ? (not just the ones immediately following the '//')
Created attachment 87049 [details] Possible solution If it helps with the assessment, I've attached a possible solution utilising regular expressions; although I appreciate you may prefer to stick with the prefix/suffix mechanism, depending on whether you perceive regexps to be too slow. Either way, this will at least allow you to run some tests.
>I would contend that simplicity does not have the same meaning here for the >implementor and for the user... I wouldn't say that having a space is simpler. We have quick fixes and the NLS tool to add/remove them. I'd says for those who still want to type that stuff in manually it's easy enough. I agree with Olivier to not change the current behavior. What would be the preferred format? //$NON-NLS-1$ //$NON-NLS-2$ // $NON-NLS-1$ // $NON-NLS-2$ //$NON-NLS-1$ $NON-NLS-2$ // $NON-NLS-1$ $NON-NLS-2$ Would we have to add a preference to set the preferred format and change all our tools (nls wizard, quick fix, nls hover) to respect that? Also there's another issue with this proposal: clients who e.g. have this: some code; // this adds he $NON-NLS-1$ tag other code; // this adds he $NON-NLS-2$ tag will get compiler warnings on their code.
Created attachment 87070 [details] Simple patch to alter the behavior of the non-externalized string warning detection I think what is being requested is to support not a different style as such, but multiple styles by being a little more relaxed in the $NON-NLS-n$ detection - i.e. not to replace "//$NON-NLS-1$" with "// $NON-NLS-1$" but to allow both. I appreciate that my earlier patch didn't deal with the detection of the tags as part of the build (i.e. when the "Non-externalised strings" compiler warning/error is set), but I'm not sure I've fully grasped the scenarios you're concerned about. For example, if the build was fixed to not display the warning/error in cases where "// $NON-NLS-1$" was used, the quick fix wouldn't be available. When the quick fix is available, it'll still put "//$NON-NLS-1$" which shouldn't be an issue. Are there other quick fixes that I've missed? I've attached another (very simplistic, perhaps naive) patch that seems to work for the limited scenario's that I've tested. Let me know if it's too simplistic and causes problems elsewhere. As regards the NLS tool, the patch that I'd supplied previously does deal with the issues of replacement and removal; certainly for the tests I've run. I'd be interested to understand scenarios where it's not performing as required. Finally, I can imagine that you guys are quite busy, so any time you have available to assess these patches is greatly appreciated.
For Info: The second patch is in addition to the first patch, not instead of.
(In reply to comment #9) > >I would contend that simplicity does not have the same meaning here for the > >implementor and for the user... > I wouldn't say that having a space is simpler. We have quick fixes and the NLS > tool to add/remove them. I'd says for those who still want to type that stuff > in manually it's easy enough. I agree with Olivier to not change the current > behavior. > > What would be the preferred format? > //$NON-NLS-1$ //$NON-NLS-2$ > // $NON-NLS-1$ // $NON-NLS-2$ > //$NON-NLS-1$ $NON-NLS-2$ > // $NON-NLS-1$ $NON-NLS-2$ > > Would we have to add a preference to set the preferred format and change all > our tools (nls wizard, quick fix, nls hover) to respect that? > > Also there's another issue with this proposal: clients who e.g. have this: > some code; // this adds he $NON-NLS-1$ tag > other code; // this adds he $NON-NLS-2$ tag > will get compiler warnings on their code. > Dani, like it or not, users do edit code, often using things like typing characters and cut and paste. Having said that, our current behavior makes '//$NON-NLS-[1-9][0-9]*$' the syntax for our tags. This is counter-intuitive, at best, especially because programmers tend to see the '$.*$' part of it as a nice, symmetric expression. (In other words, the addition of the // in front of it is awkward - at best.) May I also dare stress the fact that, while the warning reads '... it should be followed by //$NON-NLS-<n>$' and conforms to that odd syntax, the quick fix says 'Add missing $NON-NLS$ tag', failing to conform to the syntax on the one hand (missing // and missing -<n>), and reinforces the user's impression that the tag could well be '$NON-NLS-<n>$'. Regarding the preferred formats, I believe we need to be flexible here, and avoid to over-specify this. What I'd like to see is $NON-NLS-<n>$ tags in any place of a comment on the same line erase the warning for the matching string. This would be intuitive, and the fact that the tooling adds a (any) preferred form of it when performing it on behalf of the user is far less important to me. While we are at looking at that NON-NLS usability issue, here are further thoughts that came to me, for which I'll open separate bugs if needed: - if you have s = "cheers"; //$NON-NLS-1$ and you insert a line break before the semi-column, you get two warnings because the tag is on the wrong line; this verges on the side of the bug; - same remark if you cut between the two e-s; the Java editor is smart enough to add the needed double quotes and plus character, but fails to adjust the $NON-NLS-<n>$ tags; - variations of the above... - when you have several strings on the same line, a catch-all notation would help quite a lot; compare: s = "a" + s2 + "b"; //$NON-NLS-1$ //$NON-NLS-2 with: s = "a" + s2 + "b"; //$NON-NLS-*$ - s = "a"; //$NO^code assist should work; - (more controversial: nowadays, 'tag' in a Java context would naturally raise images like @something in any programmer's mind; would be nice to consider such a pattern in this context as an addition to the $.*$ pattern).
If we invest any time in this, then we should change the NON-NLS tags to be used inside block comment and then they could be placed right besides the corresponding string literals. I would not change anything unless we modify the NON-NLS support in depth.
I'm against using block comments beside each string: that would make reading the code much harder and pollute it in my opinion.
I agree with Daniel about the block comment option; for me, it would produce very cluttered and ugly code.
Any NON-NLS tag is already ugly :-)
>Any NON-NLS tag is already ugly :-) Yeah, we should have chosen the snowman character: System.out.println("\u2603"); // ☃ ;-)
I haven't tried, but I guess this change will require us to also be adapted in the NLS wizard, where we also have a scanner for 'NON-NLS' comments I personally think it's better to not extend the syntax: - the change generates ripples in the compiler, NLS tooling and quick fix - I think it's OK to ask the user to pay attention to the detail here. The matching rule is simple. - Typically, 'NON-NLS' tags are created by quick fix and the NLS tooling and not by typing and copy/paste - the change can lead to new warnings/errors if users happen to use 'NON-NLS' in their comments (Unnecessary NON-NLS tags) - the change can break other tools (unknown to us) Looking at the IMO small benefit, I don't think the invested time and risks should be taken - I agree that we should fix our warning messages in the compiler and quick fix to be consistent and correct (comment 12)
JDT now supports external formatters, some of which are unaware of the //$NON-NLS- convention. It would be more flexible if the compiler allowed spaces, and would be consistent with how the JDT formatter handles @formatter: directives.
