Skip to content

Conversation

@proski
Copy link

@proski proski commented Jul 8, 2019

*  StashBuildTrigger: Rename StashBuildTriggerDescriptor to DescriptorImpl
   
   DescriptorImpl is the standard name for a descriptor defined as an inner
   class. StashBuildTriggerDescriptor is a long name that duplicates the
   name of its containing class, StashBuildTrigger.

Actually, StashPostBuildComment.java uses DescriptorImpl, so the plugin code is inconsistent. Let's make it consistent.

The descriptor name is written to the XML file that holds the global configuration of the plugin. That's where the option to enable pipelines would go. That's why I want this PR to be merged before #69 so that we don't deal with backward compatibility later.

@proski proski mentioned this pull request Jul 8, 2019
@jakub-bochenski
Copy link

The descriptor name is written to the XML file

Don't we need a data migration then?

@proski
Copy link
Author

proski commented Jul 11, 2019

We don't have any data to migrate because we don't have any global settings yet. I actually tried upgrading from the master branch to this PR, there are no messages about unreadable data.

The goal is to have global setting use the shorter and idiomatic name from the beginning. That way we avoid data migration in the future.

DescriptorImpl is the standard name for a descriptor defined as an inner
class. StashBuildTriggerDescriptor is a long name that duplicates the
name of its containing class, StashBuildTrigger.
@jakub-bochenski
Copy link

We don't have any data to migrate because we don't have any global settings yet. I actually tried upgrading from the master branch to this PR, there are no messages about unreadable data.

OK, I was worried the empty descriptor would still generate warnings

@jakub-bochenski jakub-bochenski merged commit 6bf204f into jenkinsci:master Jul 12, 2019
@proski proski deleted the rename-descriptor branch July 12, 2019 15:46
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.

2 participants