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

Load default when appending to/removing from an unset omero.web property #2030

Merged
merged 22 commits into from Feb 5, 2014

Conversation

knabar
Copy link
Member

@knabar knabar commented Jan 28, 2014

This PR extends #1905 by loading the default value from omeroweb/settings.py when a value is appended to or removed from a previously unset omero.web.* property.

$ omero config get omero.web.server_list
$ omero config append omero.web.server_list '["test",123,"omtest"]'
$ omero config get omero.web.server_list
[["localhost", 4064, "omero"], ["test", 123, "omtest"]]

@joshmoore
Copy link
Member

Fairly long thread in chat about this which will need to get summarized, but my 2 cents are:

  • Since it seems to simplify this whole process, I can live with importing omeroweb/settings.py inside of prefs.py but I'm not for it. It's spaghetti-y and I'd rather we separated concerns in the long-run.
  • The one thing this doesn't do is handle the case of changes to the defaults. This will need to be clearly outlined in upgrade pages if any changes are made to the defaults. Basically, the property names and the default values must be considered public API and any change documented. (/cc @hflynn) This is no worse than the 4.4 situation, but it's certainly a failing that I would also like to see solved in the long-run.

Otherwise, makes sense.

@knabar
Copy link
Member Author

knabar commented Jan 29, 2014

@joshmoore Agreed that changes to the defaults are an issue. I don't see a simple fix for that other than an approach like #2028.

@knabar
Copy link
Member Author

knabar commented Jan 29, 2014

--rebased-to #2041

@joshmoore
Copy link
Member

2041 is merged. I assume this just needs a cursory once over?

@will-moore
Copy link
Member

Works fine for me. This will make settings so much nicer than they are now!
Apologies for not getting much involved in this discussion so far.
One minor comment - would be nice if 'get' when nothing is set could show you what the default is. E.g. I want to remove one item from an existing list...

$ omero config get omero.web.scripts_to_ignore         # not set, but I want to remove one....
$ omero config remove omero.web.scripts_to_ignore '"/omero/import_scripts/Populate_ROI.py"'
$ omero config get omero.web.scripts_to_ignore ["/omero/figure_scripts/Movie_Figure.py", "/omero/figure_scripts/Split_View_Figure.py", 

But this is a very minor point. Good to merge!

@joshmoore
Copy link
Member

Thanks, @will-moore. Merging.

re: "get could show defaults..." -- the major problem I see with that is there's no comparable operation with set. Might be clearly if there were another command that showed all the defaults, both from etc/omero.properties as well as lib/python/omeroweb/settings.py

joshmoore added a commit that referenced this pull request Feb 5, 2014
Load default when appending to/removing from an unset omero.web property
@joshmoore joshmoore merged commit 60d3271 into ome:develop Feb 5, 2014
@sbesson sbesson added this to the 5.1.0-m1 milestone Oct 14, 2014
@knabar knabar deleted the feature-smart-omero-web-config branch August 7, 2019 10:51
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
Projects
None yet
Development

Successfully merging this pull request may close these issues.

None yet

4 participants