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

[fix] Use ID instead of name for generated_option #851

Closed
wants to merge 1 commit into from

Conversation

@Josue-T
Copy link
Contributor

Josue-T commented Nov 23, 2019

The problem

Using the "name" for each element in the config panel is bad because it could contains a space.

Solution

Use the ID.

PR Status

Tested with synapse on this branch.

How to test

Install an app with a config panel. Try the config panel and check that each change is applied.

Validation

  • Principle agreement 0/2 :
  • Quick review 0/1 :
  • Simple test 0/1 :
  • Deep review 0/1 :
@Josue-T Josue-T requested review from Psycojoker and alexAubin Nov 23, 2019
@alexAubin

This comment has been minimized.

Copy link
Member

alexAubin commented Nov 25, 2019

Sounds legit I guess 👍 , but @Psycojoker will know better than I do

@Psycojoker

This comment has been minimized.

Copy link
Member

Psycojoker commented Dec 6, 2019

Looking at this I really don't understand how I could choose to use "name" instead of "id" when writing this code frankly...

Looking at this commit it's really weird that I've decided to change from it to name for variable generation... It's probably due to some shitty property of yunohost args format and was probably done for a good reason but 6 month later I have to admit that I really don't remember why :<

I need to dig into that.

@Psycojoker

This comment has been minimized.

Copy link
Member

Psycojoker commented Dec 6, 2019

Ok I got it, the short answer is: you are out of date with the recent modifications.

Long answer:

  • config panel and actions have been modified to use the same format than manifest.json/toml
  • manifest.json/toml is "weird" and use "name" instead of "id" for actual id, see https://github.com/YunoHost-Apps/unattended_upgrades_ynh/blob/master/manifest.json#L24
  • "id", iirc, doesn't have any sens anymore in config_panel.json/toml
  • to replace the old name you need to use "ask" instead
  • all the details are here on the format you are supposed to respect #734

Also see: https://gist.github.com/Psycojoker/c9ea335e596c4684b042812b7b0804c3#file-config_panel-toml-L11

So except if we want to switch back to the old format, we don't want this modification so I'm closing this PR (re-open if needed :))

@Psycojoker Psycojoker closed this Dec 6, 2019
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Projects
None yet
3 participants
You can’t perform that action at this time.