Skip to content

Level 7b#2

Open
sandeepbarnwal wants to merge 10 commits intomainfrom
level-7b
Open

Level 7b#2
sandeepbarnwal wants to merge 10 commits intomainfrom
level-7b

Conversation

@sandeepbarnwal
Copy link
Owner

@sandeepbarnwal sandeepbarnwal commented Oct 23, 2024

@sandeepbarnwal sandeepbarnwal marked this pull request as ready for review October 23, 2024 11:06
Comment on lines +41 to +44
@DataBoundSetter
public void setSelectedCategory(String category) {
this.selectedCategory = category;
}
Copy link

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Missing a getter.

Copy link
Owner Author

@sandeepbarnwal sandeepbarnwal Oct 23, 2024

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Adding getter resolved this. I was able to get generated script like below

onboarding name: 'sa', selectedCategory: 'b5d06f12-6446-47de-8abb-143380ed30af'

So far I didn't need to expose gettter as I was selecting the categories from dropdown
 which is populated from global config's data and selectedCategory is used within the
 class OnboardingTaskBuilder. I had not added getter as it was not required. 
Is it always recommended to add getters even if a property is not being used (atleast currently) 
outside the object's context?

Copy link

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

If you expect the snippet generator to work then you must have getters for all fields. (Or the fields themselves can be public.)

Copy link
Owner Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Okay, thanks.
PS: In my opinion, snippet generator could generate the snippet considering only the properties which are public (either via getter or public) and ignoring the private properties. That would be less confusing (atleast to newcomers).

Copy link
Owner Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Discussed with Thor and came to a conclusion tha it was failing for a case where you have data bound setter but not the getter. Private properties without public scope (getter and setter) are ignored as expected.

Comment on lines +56 to +62
## Issues

## Contributing

TODO review the default [CONTRIBUTING](https://github.com/jenkinsci/.github/blob/master/CONTRIBUTING.md) file and make sure it is appropriate for your plugin, if not then add your own one adapted from the base file

Refer to our [contribution guidelines](https://github.com/jenkinsci/.github/blob/master/CONTRIBUTING.md)

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Suggested change
## Issues
## Contributing
TODO review the default [CONTRIBUTING](https://github.com/jenkinsci/.github/blob/master/CONTRIBUTING.md) file and make sure it is appropriate for your plugin, if not then add your own one adapted from the base file
Refer to our [contribution guidelines](https://github.com/jenkinsci/.github/blob/master/CONTRIBUTING.md)

<version>${revision}${changelist}</version>
<packaging>hpi</packaging>

<name>TODO Plugin</name>

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I guess something like sandeep onboarding plugin

Copy link
Owner Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

noted

@Extension
public static final class DescriptorImpl extends BuildStepDescriptor<Builder> {

public FormValidation doCheckName(@QueryParameter String value)

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

There are a lot of those methods, maybe it makes sense to extract them to a class

Copy link
Owner Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

let me re-check

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Labels

None yet

Projects

None yet

Development

Successfully merging this pull request may close these issues.

3 participants