Conversation
| pairs.append((setting.strip(), value.strip())) | ||
|
|
||
| feature_settings: dict[str, Any] = {} | ||
| for feature, gda_name in sources.items(): |
There was a problem hiding this comment.
Here we do m*n operations over all features and all lines, it would probably be better to iterate once over all lines and update the feature settings that match the property key.
It's not likely to be a big deal as hopefully we don't hit this code that often and the files aren't that big but something to bear in mind in future when doing this sort of thing.
| def parse_value(value: str, convert_to: type | None = None) -> Any: | ||
| DEFAULT_REPLACEMENTS = {"Yes": "True", "No": "False", "true": "True", "false": "False"} | ||
|
|
||
|
|
There was a problem hiding this comment.
I don't really like this function. We call it in a number of different contexts, where we are parsing a variety of different file formats.
- java properties files when we are reading the feature settings (specifically, GDA properties files, which are a superset of java property files I believe are actually Apache Commons Configuration, which might potentially contain include directives and variable expansions, although it's unlikely that we will need to evaluate them)
- beamline parameters files - these don't have any well-documented format and are mostly key=value format but there is at least one line in them which isn't
- display configuration file which is key = value with repeating keys
- lookup tables, which are columnar
In general, the expected type of the output for these is not particularly well-defined and not guaranteed to be consistent.
There is an important caveat that isn't mentioned here which is this function is only safe because:
- we trust the input (
literal_evalis explictly documented as not being suitable for untrusted input) - We are assuming that all values being parsed here fall into either quoted strings or python-compatible literals.
- This function will happily parse dicts, lists, sets and other more complex data structures here but we are probably not expecting these to appear in our input
If we want to parse anything that might resemble an arbitrary string, this function is not suitable.
Whilst it's functional I think putting it in _converter_utils makes it tempting for people to adopt but it's a bit of a footgun.
There was a problem hiding this comment.
Are you saying there should be a different parse_value or similar for each of the examples above? I think for our uses we can maybe split it into two functions, one that attempts to convert a string to int, float or bool (beamline parameters, domain.properties), and one that converts a string to int or float (display config, most LUTs). I'm happy to refactor to avoid using literal_eval.
There was a problem hiding this comment.
I've moved parse_value into the beamline parameters module and made it private. Everything else explicitly converts to a type.
jacob720
force-pushed
the
add_feature_flag_models_2
branch
from
c261177 to
226f269
Compare
jacob720
force-pushed
the
add_feature_flag_models_2
branch
from
226f269 to
10a22d6
Compare
jacob720
force-pushed
the
add_feature_flag_models_2
branch
from
0cc66d3 to
0c90b10
Compare
| @cachedmethod(cache=operator.attrgetter("_cache")) | ||
| @cachedmethod( | ||
| cache=operator.attrgetter("_cache"), lock=operator.attrgetter("_lock") | ||
| ) |
There was a problem hiding this comment.
This was a good spot, I hadn't realised the config server used a third-party cache implementation.
I had a look through our other projects in case we use it elsewhere. Strangely mx-bluesky declares it as a dependency in pyproject.toml even though it doesn't actually use it directly.
2 participants
Part of DiamondLightSource/mx-bluesky#1629