labscript-suite / labscript-utils

Shared modules used by the 𝘭𝘒𝘣𝘴𝘀𝘳π˜ͺ𝘱𝘡 𝘴𝘢π˜ͺ𝘡𝘦. Includes a graphical exception handler, debug tools, configuration management, cross platform filepath conversions, unit conversions and custom GUI widgets.
http://labscriptsuite.org
Other
2 stars 45 forks source link

labscript_suite variable in labconfig is not used as config and is misleading #35

Closed chrisjbillington closed 4 years ago

chrisjbillington commented 4 years ago

The labscript_suite variable in labconfig used to point to the install directory. Now we don't have an install directory per se that we explicitly need to know about - the packages are expected to be somewhere in the python path and should work so long as they are. Instead we have a 'profile' directory where user data and config is stored. This profile's path is hard-coded so can't be modified anyway (and we can't make it customisable via labconfig since labconfig is in the profile directory - this is a bootstrapping problem).

The only purpose this path serves is as a variable the user can use in other configurable settings.

Therefore let's get rid of it, and introduce the convention that relative paths in labconfig variables are interpreted as relative to the profile directory.

This will mean writing a get_path method or something for LabConfig that will interpret paths this way, and callers that want paths will need to call it instead of the regular get function.

So I'll aim to add that to labconfig.py, remove the labscript_suite variable, and then grep through project repos for instances of reading the config variables which are paths.

philipstarkey commented 4 years ago

So my only concern with this is that it could be a breaking change if people have used it for their own code (for example analysis code). An example of this is here. It perhaps doesn't matter specifically for that repository because the analysis code is Python 2.7 anyway, but it's an example of what people may have done.

Is it possible to remove it from the .ini file, but insert a value for that key dynamically into the ConfigParser prior to loading the .ini file based on where the profile directory is? My understanding is that ConfigParsers can load multiple files and I think keys in the default group can be used in other loaded files? Could be wrong though...

chrisjbillington commented 4 years ago

We could do that pretty easily I think. There may be other non-configurable variables users would need to use in the future, and allowing you to refer to non-configurable variables in config files is not uncommon in other projects.

I don't think we should be beholden to backward compatibility for use cases that are not explicitly supported (like adding custom stuff to the config file). But I like this idea because it might be less confusing to have variables that are defined elsewhere, than for there to be a difference in behaviour between absolute and relative paths, which is subtle and could be easily misunderstood. In either case it should be documented, but it will be more obvious if you see a variable that isn't defined to assume it is defined elsewhere and that you can use it.

It also saves having to call a different method to get paths. The paths in the config file will already be correct. (oh is that what you meant about being backward compatible with Shaun's config file - not having to add those method calls for whatever code is reading that file - I thought you just meant not having to change the config file itself. That makes a little more sense, though it still doesn't cross the line into something I would want to maintain backward compatibility with without other reasons).

Ok so I'll:

chrisjbillington commented 4 years ago

Fixed by #45