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

Rename settings (consistency) #9

Conversation

jbostoen
Copy link
Contributor

@jbostoen jbostoen commented Dec 18, 2020

Not sure if I should finish this (probably just needs testing, perhaps remove the explicit config saving instruction).

  • removes a redundant line with a typo
  • renames the settings to only use underscores (+ handling of existing settings in the configuration file: rewrite. Any native method to delete the old settings?)
  • also, some settings were not in the module's config file. While this is not necessary, it makes it clearer to users what the possible settings are. Furthermore; it seems like some of the settings were slightly different between the demo configuration and the default-if-not-specified settings in the code.

The upgrade check could also be based on version number of the extension instead.
Actually for now it does NOT work, it seems configuration is based on what is specified "by default".
I was looking into BeforeWritingConfig, which doesn't support previous and current version. (perhaps this could be added?)
But for some reason, it also doesn't include the existing configuration yet?

jbostoen added 4 commits December 18, 2020 14:59
* rename settings for consistency
* fix mistake in variable name
* different place to update configuration. More logical place, but might still need to be moved since the version check is not possible here.
* another implementation of upgrading settings. Should work.
@Hipska
Copy link
Collaborator

Hipska commented Dec 19, 2020

If You are still working on it, please mark it as a draft. (Not in the title)

@jbostoen
Copy link
Contributor Author

@Hipska I don't think that option is available to me ( "Any user with write access to the repository can convert a pull request to a draft pull request. ")

@Hipska
Copy link
Collaborator

Hipska commented Dec 20, 2020

Yes you can, in the top right:
image

@jbostoen jbostoen marked this pull request as draft December 20, 2020 18:05
@piRGoif
Copy link
Contributor

piRGoif commented Dec 23, 2020

I agree, as said in #4 I'm not sure this is worst the effort... Actually migrating existing config would be quite a challenge !
I'll ask the team and let you know !

@piRGoif
Copy link
Contributor

piRGoif commented Dec 23, 2020

As a reference the module config param documentation : https://www.itophub.io/wiki/page?id=extensions%3Aticket-from-email#other_configuration_parameters

@piRGoif
Copy link
Contributor

piRGoif commented Jan 25, 2021

The wiki page was updated to include module parameter naming convention : Configuration parameter naming

@piRGoif
Copy link
Contributor

piRGoif commented Jan 25, 2021

We prefer to reject your proposition, as there are more risks than benefits : even if this PR includes the migration code, the benefit is low and as this is part of one of the most popular extension since a long time, any bug would have enormous consequences.

Many thanks for your efforts anyway !

@piRGoif piRGoif closed this Jan 25, 2021
@piRGoif piRGoif added wontfix This will not be worked on and removed Products team review needed labels Jan 25, 2021
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
wontfix This will not be worked on
Projects
None yet
Development

Successfully merging this pull request may close these issues.

3 participants