Mark RequiresDirective's name as MANDATORY
#3479
Conversation
datho7561
force-pushed
the
requires-should-allow-deleting-name
branch
2 times, most recently
from
2d24df1 to
1405884
Compare
|
@mpalat could you please review this code once you have a chance? From my understanding, you implemented |
There was a problem hiding this comment.
@mpalat could you please review this code once you have a chance? From my understanding, you implemented RequiresDirective, so you probably have the best understanding of it. I don't understand why the name was being lazily initialized, and I don't understand why a qualified name was used in the lazy initalization.
hmm.. well, let me scratch my old memory :)
Looking at the specs, I believe the name should have been MANDATORY rather than optional while it was modifiers that had to be optional - referring section - referring Section 7.7 of JLS :
requires {RequiresModifier} ModuleName. So it's best to change the name to mandatory and leave lazy initialization in tact.
And to answer your second question on why a qualified name was used - I don't see why it should not be used. Guess that it so coded since this mimics import. We can use either a Simple name or a QualifiedName since the ModuleName can be one of those - ref: Section 6.5. Am open for a change to SimpleName if it makes life easier in any case - but modules names tend to be qualified names mostly [no, I don;t have any statistics to prove that].
So I believe the name should be made mandatory and Javadoc updated rather than allowing null.
|
Okay sounds good. I'll update this PR to mark the name as MANDATORY and update the Javadoc |
|
I've added a new commit that marks the name as MANDATORY and switches the default name over to a SimpleName. I'm going to wait for the CI results before removing the first commit |
datho7561
force-pushed
the
requires-should-allow-deleting-name
branch
from
09ae75c to
892faa8
Compare
datho7561
changed the title
name of RequiresDirective to nullRequiresDirective's name as MANDATORY
- Switch the value of the lazily initialized name to be a `SimpleName` - Document that `RequiredDirective.setName(Name name)` throws an `IllegalArgumentException` when attempting to set the name to `null` Signed-off-by: David Thompson <davthomp@redhat.com>
datho7561
force-pushed
the
requires-should-allow-deleting-name
branch
from
892faa8 to
4dba344
Compare
|
Alright, it should be good to review: I've cleaned up the commit and added a test. |
This is new Java 23 syntax. This PR should make 6 new tests pass (mostly from the java 23 completion suite) This PR also incorporates eclipse-jdt#3479, I forget specifically why, but it's needed. - Fixes to the javac AST converter - I did a hack in the converter to recover the following case, since javac doesn't handle it currently: ```java import module public class HelloWorld { } ``` - Adjust module completion relevance based on if it's required by the current module Signed-off-by: David Thompson <davthomp@redhat.com>
Elsewhere in the code, it's treated as a mandatory property (for instance in AST rewrites), so it makes sense to update the property descriptor and Javadoc to reflect this.
What it does
RequiresDirective's name field asMANDATORYSimpleNameRequiredDirective.setName(Name name)throws anIllegalArgumentExceptionwhen attempting to set the name tonullHow to test
Author checklist