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.
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
Fix for WFCORE-2283. Completion for alternatives,requires and required #2811
Conversation
c59327d
to
6460f2a
Compare
Full integration - Windows Build 4272 outcome was FAILURE using a merge of 6460f2a Failed tests
|
6460f2a
to
a2b024e
Compare
Core - Full Integration Build 5999 outcome was FAILURE using a merge of a2b024e |
Full integration - Windows Build 4423 outcome was FAILURE using a merge of a2b024e |
Core - Full Integration Build 6015 outcome was FAILURE using a merge of a2b024e Failed tests
|
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
A couple minor questions aside, looks good. I didn't really review the change to OperationRequestCompleter because for that file the diff is just too complex. I think there it's best for me to just count on the existing tests and the new tests you've added.
assertEquals(Arrays.asList("}"), candidates); | ||
} | ||
|
||
{ |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Can you add a comment before this one noting that it's validating that the "If the radical of a property name is typed, the property name (if it exists, hidden or not) is proposed" general rule from the design doc (or some words like that)? The other test blocks are pretty self-explanatory.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Ok, done.
@@ -198,12 +198,14 @@ public void marshallAsAttribute(AttributeDefinition attribute, ModelNode resourc | |||
ModelDescriptionConstants.INPUT_STREAM_INDEX, ModelDescriptionConstants.HASH, ModelDescriptionConstants.BYTES, | |||
ModelDescriptionConstants.URL, ModelDescriptionConstants.EMPTY) | |||
.setRequires(ModelDescriptionConstants.PATH) | |||
.setRequired(false) |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
There's a separate PR that deals with this class so I expect there will be conflicts. See #2900. Since there @soul2zimate has the assigned task of fixing this metadata, I plan to work through his and get it merged and then this PR will probably need to be updated.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
That PR is merged.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
It turns out the issues you're fixing here and those Chao Wang was addressing are different. And it seems there's no conflict. What you've done in this class looks fine.
Set<String> requiring = requiredBy.get(req); | ||
if (requiring == null) { | ||
requiring = new HashSet<>(); | ||
requiredBy.put(req, requiring); |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Please ignore this if I'm wrong; i.e. it's a lot of code and there's no need to waste your time explaining how I'm wrong. ;) But I don't see any reads of the Set values stored in requiredBy. I just see at L355 "return requiredBy.get(prop.getName()) != null;". So it seems that requiredBy could just be a Set and not a map.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Yes, no need for a map. I think that I used it to help debug. Replaced.
private final Map<String, Set<String>> requiredBy = new HashMap<>(); | ||
public PropertyVisibility(boolean all, List<Property> propList, | ||
Set<String> presentProperties, String radical) { | ||
this.all = all; |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I don't see this set to 'true' anywhere. Is that planned for the future?
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Good point, it was planed at some point but removed, I removed it.
a2b024e
to
80bd61d
Compare
Full integration - Windows Build 4515 outcome was FAILURE using a merge of 80bd61d |
80bd61d
to
8cea97f
Compare
8cea97f
to
1bc11e2
Compare
@bstansberry , unit test rebased. |
Better completion for alternatives and required arguments.
Community Doc PR: wildfly/wildfly#10798