Skip to content
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

[JENKINS-29922] mark meta step #6

Merged
merged 20 commits into from Jul 28, 2016

Conversation

Projects
None yet
6 participants
@kohsuke
Copy link
Member

kohsuke commented Jun 29, 2016

Documenting the contract of a meta step

@jglick jglick closed this Jul 11, 2016

@jglick jglick reopened this Jul 11, 2016

kohsuke added some commits Jul 11, 2016

[JENKINS-29922] made UninstantiatedDescribable aware
This change is needed to make snippetizer able to leverage symbols
I think "uninstantiate" is a better name here
... because it's consistent with DescribableModel.uninstnatiate()
@kohsuke

This comment has been minimized.

Copy link
Member Author

kohsuke commented Jul 11, 2016

This PR and other JENKINS-29922 PRs are ready to merge: @reviewbybees

@kohsuke kohsuke changed the title [WiP] [JENKINS-29922] mark meta step [JENKINS-29922] mark meta step Jul 11, 2016

@reviewbybees

This comment has been minimized.

Copy link

reviewbybees commented Jul 11, 2016

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.

@abayer

This comment has been minimized.

Copy link
Member

abayer commented Jul 11, 2016

🐝

@@ -93,6 +104,79 @@ public boolean isAdvanced() {
}

/**
* Some steps, such as {@code CoreStep} or {@code GenericSCMStep} can take

This comment has been minimized.

Copy link
@jglick

jglick Jul 12, 2016

Member

But you have no PR modifying GenericSCMStep, do you?

This comment has been minimized.

Copy link
@kohsuke

kohsuke Jul 12, 2016

Author Member

Can we do that in a follow up change? There's some additional complication around how to migrate svn and git steps, too.

This comment has been minimized.

Copy link
@jglick

jglick Jul 12, 2016

Member

Well, but other SCM implementations should work right away.

* public String getFunctionName() {
* return "metaStepForFoo";
* }
* }

This comment has been minimized.

Copy link
@jglick

jglick Jul 12, 2016

Member

This should also show you overriding isMetaStep!

*
* <pre>
* public class Xyz extends Foo {
* &#64;DataBoundConstructor

This comment has been minimized.

Copy link
@jglick

jglick Jul 12, 2016

Member

IIRC if you just wrap this all in {@code …} you do not need to bother with such escapes.

This comment has been minimized.

Copy link
@kohsuke

kohsuke Jul 12, 2016

Author Member

I just tried but it doesn't handle multiple lines

This comment has been minimized.

Copy link
@jglick

jglick Jul 12, 2016

Member

Yes it does!

This comment has been minimized.

Copy link
@jglick

jglick Jul 26, 2016

Member

Huh, I swore it did, but it seems it does not.

This comment has been minimized.

Copy link
@johnou

johnou Jul 26, 2016

Member

@jglick @kohsuke it does, however if you use curly braces "{}" in your example code the first closing brace will terminate the @code block :(

This comment has been minimized.

Copy link
@jglick

jglick Jul 27, 2016

Member

Right, I found a lengthy list of answers on StackOverflow about it.

This comment has been minimized.

Copy link
@johnou

johnou Jul 27, 2016

Member

Sounds like we read the same page.

This comment has been minimized.

* For a {@linkplain #isMetaStep() meta step}, return the type that this meta step handles.
* Otherwise null.
*/
// not @CheckForNull because often the caller is using allMeta()

This comment has been minimized.

Copy link
@jglick

jglick Jul 12, 2016

Member

Then use @Nullable.

This comment has been minimized.

Copy link
@kohsuke

kohsuke Jul 12, 2016

Author Member

I don't think adding @Nullable to return value to anything as far as tools are concerned.

This comment has been minimized.

Copy link
@jglick

jglick Jul 13, 2016

Member

It is better for documentation purposes.

LOGGER.log(Level.WARNING, getClass()+" claims to be a meta-step but it has no parameter in @DataBoundConstructor");
// don't punish users for a mistake by a plugin developer. return null instead of throwing an error.
// returning a type that doesn't match anything normally prevents this broken StepDescriptor from getting used.
return Void.class;

This comment has been minimized.

Copy link
@jglick

jglick Jul 12, 2016

Member

Would not null be more appropriate?

This comment has been minimized.

Copy link
@jglick

jglick Jul 12, 2016

Member

(the line above claims you are returning null, but then you do not)

This comment has been minimized.

Copy link
@jglick

jglick Jul 12, 2016

Member

And do you really mean Void.class, which does match a null reference, rather than Void.TYPE ~ void.class, which is the bottom type?

@@ -109,9 +193,33 @@ public Step newInstance(Map<String,Object> arguments) throws Exception {
* @param step a fully-configured step (assignable to {@link #clazz})
* @return arguments that could be passed to {@link #newInstance} to create a similar step instance
* @throws UnsupportedOperationException if this descriptor lacks the ability to do such a calculation
* @deprecated

This comment has been minimized.

Copy link
@jglick

jglick Jul 12, 2016

Member

Also add @Deprecated.

This comment has been minimized.

Copy link
@kohsuke

kohsuke Jul 12, 2016

Author Member

Added

return DescribableModel.uninstantiate_(step);
if (Util.isOverridden(StepDescriptor.class, getClass(), "uninstantiate", Step.class)) {
// if the subtype has defined the uninstantiate() method, delegate to that
return uninstantiate(step).toMap();

This comment has been minimized.

Copy link
@jglick

jglick Jul 12, 2016

Member

This is not really going to work, is it? Because it will be returning a map with entries containing UninstantiatedDescribable values; whereas callers of this method will be expecting entries containing Map values.

This comment has been minimized.

Copy link
@kohsuke

kohsuke Jul 12, 2016

Author Member

UninstantiatedDescribable.toMap() recursively replaces UninstantiatedDescribable to a map thereby correctly implementing the expected semantics.

public UninstantiatedDescribable uninstantiate(Step step) throws UnsupportedOperationException {
if (Util.isOverridden(StepDescriptor.class, getClass(), "uninstantiate", Step.class)) {
// newer clients are called older implementations
return new UninstantiatedDescribable(defineArguments(step));

This comment has been minimized.

Copy link
@jglick

jglick Jul 12, 2016

Member

Again this does not look right. You would need to translate Map values to UninstantiatedDescribable values, right?

This comment has been minimized.

Copy link
@kohsuke

kohsuke Jul 12, 2016

Author Member

We can, but doing so doesn't buy us much, so we don't need to.

a literal map is a valid part of the object graph in UninstantiatedDescribable, and we'll just end up producing that with a map entry named "$class". We cannot reliably look at such a map and recreate a UninstantiatedDescribable without knowing the expected type.

We talked about this earlier and we said it's OK for legacy step implementations to behave poorly wrt snippetizer, the primary client of this functionality.

This comment has been minimized.

Copy link
@jglick

jglick Jul 13, 2016

Member

OK but best to put this explanation in comments so the tradeoff is clear.

return Iterables.filter(all(), new Predicate<StepDescriptor>() {
@Override
public boolean apply(@Nullable StepDescriptor i) {
return i!=null && i.isMetaStep();

This comment has been minimized.

Copy link
@jglick

jglick Jul 12, 2016

Member

Huh? How could ExtensionList.lookup be returning null elements?

This comment has been minimized.

Copy link
@kohsuke

kohsuke Jul 12, 2016

Author Member

This is another "let's keep findbugs happy exercise"

This comment has been minimized.

Copy link
@kohsuke

kohsuke Jul 12, 2016

Author Member

I tried a few variants just now like (1) not having annotation, (2) switch it to @Nonnull etc but either my IDE is unhappy or findbugs is unhappy.

This comment has been minimized.

Copy link
@jglick

jglick Jul 12, 2016

Member

@SuppressFBWarning if it is giving a false positive.

This comment has been minimized.

Copy link
@jglick

jglick Jul 12, 2016

Member

Pending -source 8 we cannot annotate type parameters on the return value of lookup.

kohsuke added some commits Jul 12, 2016

Clarifying the comment.
(Void.class vs Void.TYPE doesn't make much of a practical difference)
@svanoort

This comment has been minimized.

Copy link
Member

svanoort commented Jul 27, 2016

🐝 AFAICT

@abayer

This comment has been minimized.

Copy link
Member

abayer commented Jul 27, 2016

🐝

1 similar comment
@kohsuke

This comment has been minimized.

Copy link
Member Author

kohsuke commented Jul 27, 2016

🐝

@jglick jglick merged commit 1bb710c into master Jul 28, 2016

1 check passed

Jenkins This pull request looks good
Details

@jglick jglick deleted the JENKINS-29922 branch Jul 28, 2016

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
You can’t perform that action at this time.