Comments (15)
Hi Thomas, can you take a look at this?
from eclipse.jdt.ui.
@vik-chand which cleanup and/or quick fix is this? Instructions to reproduce would be much appreciated.
from eclipse.jdt.ui.
There are a couple of factors to be considered here:
- The "non-nls" warning is not a Java error, instead it's a convention introduced by the Eclipse source code itself
- The problem is configurable and might be turned off. In this case I would consider it wrong to introduce "$NON-NLS" tags.
- There might be tagged strings outside of the initializer being moved both in the origin as well as the target line. Both existing and moved tag numbers will have to be updated. Consider this case:
String bla= "gork";File f= new File("abc"); String zoz= "zoz"; //$NON-NLS3 f= new File("foo" + "bar"); //$NON-NLS2
- If tags exist, they should be moved, even if omitting them would not introduce an error (as in case 2.)
from eclipse.jdt.ui.
We also would need to consider that the cleanup/quickfix should also not add extra warnings.
from eclipse.jdt.ui.
The NLS comment should not be removed in the simple example originally given in the issue. You can reconstruct the comment if you create a brand new statement via ASTRewrite.createStringPlaceHolder and cast it to be a line comment. If there is some really complex situation you can think of where the cleanup won't know what to do or the code will be overly complex, simply bail on the cleanup (e.g. don't handle any VarDeclarationStatement with multiple fragments, don't handle multiple statements on a single line, etc..). There should be few of these complex scenarios in the wild but the first scenario is very common.
from eclipse.jdt.ui.
@jjohnstn a problem I'm running into is that the AST simply does not seem to have any line information: if there already is a line comment on the declaration line, how do I even match that to the var initializer I'm replacing? NLS processing is based on TextEdits and lines, not an AST.
from eclipse.jdt.ui.
Btw: I think even before I started working on this, the cleanup could have introduced warnings. Consider this:
var foo= new File("abcdef"); //$NON-NLS-1
foo= new File(this.currentFile);
That would have been rewritten to
var foo= new File(this.currentFile); //$NON-NLS-1
, which will generate a "unnessary non-nls tag" warning.
from eclipse.jdt.ui.
Hi Thomas, there are two methods you can call: ASTNodes.getLeadingComments(node) and ASTNode.getTrailingComments(node).
From there, you can build a statement in a StringBuffer and prepend/append comments as you please. You can then cast that String into any node using ASTRewrite.createStringPlaceHolder(String s, int nodeType). You can see an example of this in: SwitchExpressionsFixCore.getNewStatementForCase(). In that example, I move some leading line comments into a switch case so line comments need to be converted to block comments so they don't obliterate the line.
from eclipse.jdt.ui.
I've thought about this intensely, and handling this gets fiendishly difficult except for the most trivial case: where both the original and the overriden assignment sit alone on their own line. In any other case it's not even clear, where one would insert a line comment: from a syntactical point of view, comments are empty space, so you'd have to find an insert location that is between two ast nodes that are on the current and the next line. The location may also be the first child of an "upper level" node, etc.
As I said, this gets hairy real fast.
from eclipse.jdt.ui.
Handle the cases you can. That is the vast majority of cases. Who declares a variable with an initializer, then changes its value on the same line AND has comments intertwined? If you don't add the NLS for the simple cases, this will break builds and create a lot of work for the end-user. If a user has some convoluted situation where the original and overridden are on the same line (easy to recognize) and there are comments, simply bail; don't do the cleanup.
from eclipse.jdt.ui.
I'm not talking about strange cases, but simple ones like these:
String foo= "bar" + //$NON-NLS-1
"zoz" +
"gorx"; // NON-NLS-1
or
var z= "borx"; var x= "foo"; // some comment, maybe for the first, maybe for the second var //$NON-NLS-1 //$NON-NLS-2
...
x= "bla";
or even worse:
if ("TRUE".equals(str)) { var x= "bar"; //$NON-NLS-1 //NON-NLS-2
Even just detecting that this last case is a problem is not trivial looking at the AST. There is a good reason the NLS fixes work on text edits, not AST manipulation. Mixing the two is hard.
That said, yes, I'll just bail on anything that is non-trivial. But if our aim is to not introduce any warnings/errors, we need to detect all the cases where we would introduce a problem, which itself is non-trivial.
from eclipse.jdt.ui.
This is driving me nuts! So I've got a solution that would just move the line end comments: the problem is: line comments are not part of the AST, so you can't remove them from the AST either (throws NPE). So the only solution I can see would be to replace the parent node of the variable declarations (probably a Block
) as text. But that does not seem like a good approach: we'd have to do the manipulation of the AST as well as text edits (instead of AST manipulation).
At this point, I would close this one as "won't fix", t.b.h.
from eclipse.jdt.ui.
@jjohnstn I tried your idea with using string placeholders to replace the whole variable declaration statement. The problem I'm running into is in handling the leading comments. For example:
/**
* Some comment
*/
String bla= "foo" //$NON-NLS-1
If I use a ASTRewrite.createStringPlaceholder()
, I can either lost the leading comment or the leading indent of the comment gets scrambled in the result since string placeholders do not get formatted. Should I try to fix the indenting of leading comments myself or what's a promising approach here?
from eclipse.jdt.ui.
Never mind, I found a way to ignore the leading comments using a custom source range computer.
from eclipse.jdt.ui.
After looking at it, I think it is better to ensure that both the declaration and replacement lines don't share lines with other statements rather than restrict if they have multiple lines. It is fine to move up the case with:
int s = "some line"; //$NON-NLS-1$
s = "abc" + //$NON-NLS-1$
"def" +
"ghi"; //$NON-NLS-1$
as this doesn't cause any warnings that weren't already there. I have modified the code and added a new case to the test that verifies that movement doesn't occur when there are multiple statements on a line or multiple declarations on a line. I also modified to move preceding comments as well which is in the test as well.
from eclipse.jdt.ui.
Related Issues (20)
- [Bug][Move Static Memebers Refactoring] Move static members refactoring results in behavior change
- [Bug][Rename Refactoring] Avoid the perform of rename refactoring at the implicit enum elements (e.g. values() method) HOT 4
- [Bug][Pull Up Refactoring] Pull up refactoring produces uncompilable program
- [Bug][Pull Up Refactoring] Pull up method refactoring for method in the inner class fails
- Clean Up produces compile error for JUnit5 assertion (possibly generics compile issue)
- [Bug] [Folding mechanism] Method declaration on the Same Line as the Closing Bracket
- [23] Partition support for markdown comments HOT 1
- Convert to Text-Block quick-fix/clean-up not applicable for StringBuilder/Buffer concatenation HOT 2
- Quickfix for return type that's a wildcard doesn't work post 1.4 HOT 1
- Generating hashCode method on a child class fails to fully-qualify field names
- Invalid layout for Compare control in Refactor dialog
- 5 jdt.ui and jdt.refactoring tests fail since I20240801-1800 HOT 7
- Update InlineTempRefactoring logic to properly add method type parameters when needed
- UI freeze when saving big file after find replace HOT 2
- Javadoc view make method parameter type's style to be hovered by default
- javadoc: Explicit link to package without package-info.java does not work HOT 2
- [23] Merge master branch into BETA_JAVA23 branch
- JEP 467 adjust heading levels? HOT 1
- [23] Compiler compliance level 23 missing (BETA) label HOT 2
- [23] Highlight 'module' in module imports
Recommend Projects
-
React
A declarative, efficient, and flexible JavaScript library for building user interfaces.
-
Vue.js
🖖 Vue.js is a progressive, incrementally-adoptable JavaScript framework for building UI on the web.
-
Typescript
TypeScript is a superset of JavaScript that compiles to clean JavaScript output.
-
TensorFlow
An Open Source Machine Learning Framework for Everyone
-
Django
The Web framework for perfectionists with deadlines.
-
Laravel
A PHP framework for web artisans
-
D3
Bring data to life with SVG, Canvas and HTML. 📊📈🎉
-
Recommend Topics
-
javascript
JavaScript (JS) is a lightweight interpreted programming language with first-class functions.
-
web
Some thing interesting about web. New door for the world.
-
server
A server is a program made to process requests and deliver data to clients.
-
Machine learning
Machine learning is a way of modeling and interpreting data that allows a piece of software to respond intelligently.
-
Visualization
Some thing interesting about visualization, use data art
-
Game
Some thing interesting about game, make everyone happy.
Recommend Org
-
Facebook
We are working to build community through open source technology. NB: members must have two-factor auth.
-
Microsoft
Open source projects and samples from Microsoft.
-
Google
Google ❤️ Open Source for everyone.
-
Alibaba
Alibaba Open Source for everyone
-
D3
Data-Driven Documents codes.
-
Tencent
China tencent open source team.
from eclipse.jdt.ui.