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

config is built using ordered dict #1800

Closed
wants to merge 1 commit into from
Closed

Conversation

@lukh
Copy link
Contributor

lukh commented Sep 16, 2019

Configuration is based on ordered dict, allowing apps to provide better settings UI and user experience (like web-settings or equivalent)

@codecov

This comment has been minimized.

Copy link

codecov bot commented Sep 16, 2019

Codecov Report

Merging #1800 into develop will not change coverage.
The diff coverage is 100%.

Impacted file tree graph

@@           Coverage Diff            @@
##           develop    #1800   +/-   ##
========================================
  Coverage    81.81%   81.81%           
========================================
  Files           77       77           
  Lines         6323     6323           
========================================
  Hits          5173     5173           
  Misses        1150     1150
Impacted Files Coverage Δ
mopidy/config/schemas.py 92.1% <100%> (ø) ⬆️

Continue to review full report at Codecov.

Legend - Click here to learn more
Δ = absolute <relative> (impact), ø = not affected, ? = missing data
Powered by Codecov. Last update 1ffe462...ea9d376. Read the comment docs.

@kingosticks

This comment has been minimized.

Copy link
Member

kingosticks commented Sep 16, 2019

Could you give an example of why this is useful?

@lukh

This comment has been minimized.

Copy link
Contributor Author

lukh commented Sep 16, 2019

Hello,
I am currently working on an extension for mopidy, that is the main "frontend" for my application.
I still want to let the user to configure the whole mopidy options, and having the options sorted (basically following the developper idea) make it easier to read. (the extension get the config dict, and use it as it is)

@kingosticks

This comment has been minimized.

Copy link
Member

kingosticks commented Sep 16, 2019

basically following the developper idea

By this, do you mean that the most 'important' options are ordered before the others e.g. enabled, username, passwords etc?

@lukh

This comment has been minimized.

Copy link
Contributor Author

lukh commented Sep 16, 2019

yes, that's it ! it follows the order that was implemented in get_config_schema
The developper has to order it, but you can "group" params:
user
passwd
serial_port
serial_baudrate
noise_folder

the front end can keep this order because of ordered dict

@kingosticks

This comment has been minimized.

Copy link
Member

kingosticks commented Sep 16, 2019

We probably need to think again about an API for modifying the config. Exposing secret values to all extensions isn't very nice and nor is this kind of thing.

However, for now I guess this is OK. And in python3 the dictionary will be ordered anyway.

And FYI, in mopidy-websettings we found that you don't normally want to expose changing all the config since some of it is not interesting to most people. And in order to produce a sensible html form you need some knowledge of what type each config value is. So for both those reasons we ended up explicity defining what config to show and in what order, the types, and some simple validation rules.

@lukh

This comment has been minimized.

Copy link
Contributor Author

lukh commented Sep 18, 2019

kingosticks,
Well actually you are right.. I'll end up letting the UI chooses what parameters to expose, and therefore in a specified order...
And yeah, I meet with the type of data (list, string, int, bool, none, etc) problem..
Should we keep this PR ? It looks useless then

@kingosticks

This comment has been minimized.

Copy link
Member

kingosticks commented Sep 18, 2019

I reckon we can close it.

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