Symbol annotations on core #2293

Merged
merged 26 commits into from May 4, 2016

Conversation

8 participants
@kohsuke
Member

kohsuke commented Apr 26, 2016

(Continuing on from #2160)

To help structs-plugin and its users (such as pipeline, system-config-dsl, etc), we should have core classes annotated with @Symbol

What changed from #2160?

  • Targetting master branch, which is where the action is now.
  • Additional symbol names are attached for f94ce3a

What hasn't changed from #2160?

  • Not pulling out changes that modify static field Descriptor instantiation like this
  • Not removing @Symbol on relatively minor extensions. I'd rather have one sweep to add this to wherever it can and avoid discovering few months down the road that the annotation was missing on this one class and it's 3 months away until the one line fix gets rolled into LTS
  • @Symbol is on extensions, therefore it is on Descriptor and not on Describable. One instance where this got problematic is ParameterValue and we'll fix that by making ParameterValue nominally Describable, like we often do in similar cases (see example)
  • Jenkins.setViews() is unrelated and not atomic, which is OK for the use case. I clarified that in javadoc.

@reviewbybees

kohsuke added some commits Dec 22, 2015

make the first view primary if none is chosen
... but some views already exist.
More symbol assignments and constructor diet
to move more properties off to setters.
Adding @Symbol
Where applicable symbol names used by Job DSL plugin is used for maximum compatibility: https://jenkinsci.github.io/job-dsl-plugin/
Merge remote-tracking branch 'origin/2.0' into symbol
Conflicts:
	core/src/main/java/jenkins/slaves/JnlpSlaveAgentProtocol.java
	core/src/main/java/jenkins/slaves/JnlpSlaveAgentProtocol2.java
Merge remote-tracking branch 'origin/master' into symbol
Conflicts:
	core/src/main/java/hudson/model/FreeStyleProject.java
	core/src/main/java/jenkins/model/GlobalConfigurationCategory.java
@reviewbybees

This comment has been minimized.

Show comment
Hide comment
@reviewbybees

reviewbybees Apr 26, 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.

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.

@kohsuke

This comment has been minimized.

Show comment
Hide comment
@kohsuke

kohsuke Apr 26, 2016

Member

Calling out @daspilker as per requested.

Wrt xxxParam, I don't think we need to change it unless somebody brings that as a proposal. I have no problem with the current name, and as you say it solves the issue for boolean. I think reviewers have pointed it out just because the motivation wasn't obvious when looking at the diff.

Member

kohsuke commented Apr 26, 2016

Calling out @daspilker as per requested.

Wrt xxxParam, I don't think we need to change it unless somebody brings that as a proposal. I have no problem with the current name, and as you say it solves the issue for boolean. I think reviewers have pointed it out just because the motivation wasn't obvious when looking at the diff.

@kohsuke

This comment has been minimized.

Show comment
Hide comment
@kohsuke

kohsuke Apr 27, 2016

Member

Turns out the test failure requires a small change in stapler/stapler#74

Member

kohsuke commented Apr 27, 2016

Turns out the test failure requires a small change in stapler/stapler#74

@daspilker

This comment has been minimized.

Show comment
Hide comment
@daspilker

daspilker Apr 27, 2016

Member

The Symbol JavaDoc says "The symbol must be a valid Java identifier.", but boolean is not a valid identifier.

The Symbol JavaDoc says "The symbol must be a valid Java identifier.", but boolean is not a valid identifier.

This comment has been minimized.

Show comment
Hide comment
@jglick

jglick May 4, 2016

Member

Maybe the Javadoc should be amended to say that the primary symbol must be a valid Java identifier, but that non-identifiers may be used as aliases.

Member

jglick replied May 4, 2016

Maybe the Javadoc should be amended to say that the primary symbol must be a valid Java identifier, but that non-identifiers may be used as aliases.

@daspilker

This comment has been minimized.

Show comment
Hide comment
@daspilker

daspilker Apr 27, 2016

Member

+1 for adding the symbols.

The next Job DSL release will make use of symbols and the structs plugin. I tested this branch and it works well with Job DSL. I found some issues (JENKINS-34105, JENKINS-34464), but nothing critical.

Is there some plan for further improvements in this area, e.g. sorting out mandatory vs optional parameters (aka @DataBoundConstructor vs @DataBoundSetter)? E.g. the defaultValue and description parameters are @DataBoundConstructor parameters for most ParameterDefinitions, so they are mandatory, but should not. Fixing this is IMHO an important second step for enabling concise scripts after adding the symbols.

Member

daspilker commented Apr 27, 2016

+1 for adding the symbols.

The next Job DSL release will make use of symbols and the structs plugin. I tested this branch and it works well with Job DSL. I found some issues (JENKINS-34105, JENKINS-34464), but nothing critical.

Is there some plan for further improvements in this area, e.g. sorting out mandatory vs optional parameters (aka @DataBoundConstructor vs @DataBoundSetter)? E.g. the defaultValue and description parameters are @DataBoundConstructor parameters for most ParameterDefinitions, so they are mandatory, but should not. Fixing this is IMHO an important second step for enabling concise scripts after adding the symbols.

@KostyaSha

This comment has been minimized.

Show comment
Hide comment
@KostyaSha

KostyaSha Apr 27, 2016

Member

👎 imho it should be enough to extend field in existing @Extension without adding new plugins into core distribution.

Member

KostyaSha commented Apr 27, 2016

👎 imho it should be enough to extend field in existing @Extension without adding new plugins into core distribution.

@@ -25,7 +26,7 @@
* @author Kohsuke Kawaguchi
* @since 1.467
*/
-@Extension
+@Extension @Symbol("cli")

This comment has been minimized.

@oleg-nenashev

oleg-nenashev Apr 27, 2016

Member

How does it handle duplicated symbols?

@oleg-nenashev

oleg-nenashev Apr 27, 2016

Member

How does it handle duplicated symbols?

This comment has been minimized.

@kohsuke

kohsuke Apr 29, 2016

Member

See @Symbol javadoc that clarifies this. (cc:@kzantow)

@kohsuke

kohsuke Apr 29, 2016

Member

See @Symbol javadoc that clarifies this. (cc:@kzantow)

This comment has been minimized.

@oleg-nenashev

oleg-nenashev May 3, 2016

Member

Uhh, there is some fun ahead when we start getting into conflicts

@oleg-nenashev

oleg-nenashev May 3, 2016

Member

Uhh, there is some fun ahead when we start getting into conflicts

@@ -235,7 +236,7 @@ public void restoreTo(AbstractBuild<?,?> owner, FilePath dst, TaskListener liste
}
}
- @Extension
+ @Extension @Symbol("default")

This comment has been minimized.

@kzantow

kzantow Apr 27, 2016

Contributor

default is also a reserved keyword in Java (which makes it an invalid identifier, of course). There are a few instances of @Symbol("default") in this PR, also. I'm curious about @oleg-nenashev 's question regarding duplicated symbol names.

@kzantow

kzantow Apr 27, 2016

Contributor

default is also a reserved keyword in Java (which makes it an invalid identifier, of course). There are a few instances of @Symbol("default") in this PR, also. I'm curious about @oleg-nenashev 's question regarding duplicated symbol names.

@kohsuke

This comment has been minimized.

Show comment
Hide comment
@kohsuke

kohsuke Apr 29, 2016

Member

Is there some plan for further improvements in this area, e.g. sorting out mandatory vs optional parameters (aka @DataBoundConstructor vs @DataBoundSetter)? E.g. the defaultValue and description parameters are @DataBoundConstructor parameters for most ParameterDefinitions, so they are mandatory, but should not. Fixing this is IMHO an important second step for enabling concise scripts after adding the symbols.

@daspilker, I haven't really thought about the further improvement beyond this initial round, but that one item you brought up makes a lot of sense. This PR has some changes in that area already that I have hit. I think you have some thoughts about what those further improvements should be. Can you send that as an email to the dev list so that we can discuss it there?

Member

kohsuke commented Apr 29, 2016

Is there some plan for further improvements in this area, e.g. sorting out mandatory vs optional parameters (aka @DataBoundConstructor vs @DataBoundSetter)? E.g. the defaultValue and description parameters are @DataBoundConstructor parameters for most ParameterDefinitions, so they are mandatory, but should not. Fixing this is IMHO an important second step for enabling concise scripts after adding the symbols.

@daspilker, I haven't really thought about the further improvement beyond this initial round, but that one item you brought up makes a lot of sense. This PR has some changes in that area already that I have hit. I think you have some thoughts about what those further improvements should be. Can you send that as an email to the dev list so that we can discuss it there?

@kohsuke

This comment has been minimized.

Show comment
Hide comment
@kohsuke

kohsuke Apr 29, 2016

Member

@KostyaSha I considered that but rejected it. Making this a part of @Extension creates a real hindrance when we try to put symbols on plugins that define extensions, as it requires the plugin to rely on cutting-edge versions of the core. Note that this change does not add a new bundled plugin. It merely adds a new library jar file to the core.

Member

kohsuke commented Apr 29, 2016

@KostyaSha I considered that but rejected it. Making this a part of @Extension creates a real hindrance when we try to put symbols on plugins that define extensions, as it requires the plugin to rely on cutting-edge versions of the core. Note that this change does not add a new bundled plugin. It merely adds a new library jar file to the core.

kohsuke added some commits Apr 29, 2016

'default' is not a valid identifier, so we need something else.
This is truly unfortunate because 'default' is how we call them in the
UI. My choice is between something like 'default_' vs a synonym, so I
went for 'standard'
@kohsuke

This comment has been minimized.

Show comment
Hide comment
@kohsuke

kohsuke Apr 29, 2016

Member

Addressed the problem in the symbol names default and boolean.

Member

kohsuke commented Apr 29, 2016

Addressed the problem in the symbol names default and boolean.

@rsandell

This comment has been minimized.

Show comment
Hide comment
@rsandell

rsandell May 3, 2016

Member

🐝

Member

rsandell commented May 3, 2016

🐝

@jglick

This comment has been minimized.

Show comment
Hide comment
@jglick

jglick May 4, 2016

Member

🐝

Member

jglick commented May 4, 2016

🐝

@kohsuke kohsuke merged commit 31ad770 into master May 4, 2016

1 check passed

Jenkins This pull request looks good
Details

@kohsuke kohsuke deleted the symbol branch May 4, 2016

@daniel-beck daniel-beck referenced this pull request Nov 23, 2017

Closed

Jenkins 48018 #3152

2 of 3 tasks complete
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment