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

Allow implementations do initialisation additions #121

Merged
merged 1 commit into from
Dec 4, 2018

Conversation

KostyaSha
Copy link
Member

@KostyaSha KostyaSha commented Sep 26, 2018

@jglick is there any blocker to make it non-final?
super.init() is not final but this override blocks ability to make aditional initialisations right after job was moved and has new parent, name.


This change is Reviewable

Copy link
Member

@oleg-nenashev oleg-nenashev left a comment

Choose a reason for hiding this comment

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

lgtm, I do not see specific reason to prohibit overrides. It would be great to have a @CallSuper annotation, but there is no such annotation in Java

@KostyaSha
Copy link
Member Author

KostyaSha commented Sep 27, 2018

It's also interesting that it possible to call doSetName() but there is no method to set parent in AbstractItem from child classes.

@KostyaSha
Copy link
Member Author

@jglick CC according to history you introduced final

@fcojfernandez
Copy link
Member

Hi @KostyaSha. I neither do see any specific reason to avoid the override, but since this final statement has been there since 3 years ago and nothing seems to be failing, I would like to have more details about the motivation/concerns before merging anything.

@KostyaSha
Copy link
Member Author

KostyaSha commented Nov 5, 2018

@fcojfernandez in last 3 years nobody were making any implementations based on folders, since multibranch api become wider used, custom implementations must have ability to do initialisation of their own stuff.

I would like to have more details about the motivation/concerns before merging anything.

I need to do custom things before multibranch starts. That's why upper hierarchy had no final statements. That's look like @jglick mistakenly locked it.

@fcojfernandez
Copy link
Member

But in your custom implementation you could initialise their own stuff in the constructor, since you should create one.

(Sorry if I seem picky, but I like having all the details and context to be sure the change makes sense and is safe)

@KostyaSha
Copy link
Member Author

initialisations right after job was moved and has new parent, name.

Move doesn't call constructor, move calls onLoad()

@KostyaSha
Copy link
Member Author

com.cloudbees.hudson.plugins.folder.AbstractFolder#onLoad

@jglick
Copy link
Member

jglick commented Nov 14, 2018

It would be great to have a @CallSuper annotation, but there is no such annotation in Java

javax.annotation.OverridingMethodsMustInvokeSuper

@fcojfernandez fcojfernandez merged commit 2336753 into jenkinsci:master Dec 4, 2018
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.

4 participants