JENKINS-37403 - downstream change to use SymbolLookup.getSymbolValue(…) #43
Conversation
|
This pull request originates from a CloudBees employee. At CloudBees, we require that all pull requests be reviewed by other CloudBees employees before we seek to have the change accepted. If you want to learn more about our process please see this explanation. |
| if (s != null) { | ||
| symbols.addAll(Arrays.asList(s.value())); | ||
| symbols.addAll(Arrays.asList(s)); |
There was a problem hiding this comment.
🐛 this removes support for multiple symbols.
There was a problem hiding this comment.
(Which is something you need to think about in the other downstream PRs. What is your behavior when there are multiple allowed symbols? Probably rather than @CheckForNull String you need to return @Nonnull Set<String>.)
There was a problem hiding this comment.
Ah, good point. Never thought about that! Reworking to come.
There was a problem hiding this comment.
Actually, hmm...might we actually want both? Not sure...
There was a problem hiding this comment.
Nah, I take that back. I'm overthinking. =)
| if (s != null) { | ||
| symbols.addAll(Arrays.asList(s.value())); | ||
| Set<String> s = SymbolLookup.getSymbolValue(e); | ||
| if (!s.isEmpty()) { |
There was a problem hiding this comment.
Not really necessary; could just write
symbols.addAll(SymbolLookup.getSymbolValue(e));
|
🐝 |
|
re:bee: |
|
@reviewbybees done |
|
This pull request has completed our internal processes and we now respectfully request the maintainers of this repository to consider our proposal contained within this pull request for merging. |
Fixes jenkinsci#42: Compilation threw NPE on abstract methods
JENKINS-37403
Downstream of jenkinsci/structs-plugin#10
cc @reviewbybees esp @jglick