Convert OptionStore from a dict to a full class with named methods #13307
Conversation
jpakkane
force-pushed
the
optstorerefactor
branch
from
8b5de26
to
fb4fb08
Compare
jpakkane
force-pushed
the
optstorerefactor
branch
from
fb4fb08
to
07d4c12
Compare
|
Now passes all tests on my machine. Will almost certainly fail on others, especially for mypy & lint. |
jpakkane
force-pushed
the
optstorerefactor
branch
10 times, most recently
from
ae72298
to
f0fa698
Compare
|
Ignoring linter checks this is now mostly working. The Cuda error is a bit strange and I don't even have a Cuda toolchain installed for local testing. Maybe @SoapGentoo would have an idea? |
|
I'm not seeing an obvious point of failure. Why have you replaced the direct indexing syntax though? |
Because indexing is used both for "read the value of this thingy" and "set the value of this thingy" so it is impossible to tell where in the code values are written and where they are only read. That is needed as a prerequisite for the option refactor MR. |
jpakkane
force-pushed
the
optstorerefactor
branch
3 times, most recently
from
076099c
to
0a70d30
Compare
|
Yes! This should fix all functional code. I still need to fix typing issues, but other than that this should be ready for review. |
| @@ -473,8 +474,66 @@ def add_to_argparse(self, name: str, parser: argparse.ArgumentParser, help_suffi | |||
| OptionKey('purelibdir', module='python'): {}, | |||
| } | |||
|
|
|||
|
|
|||
| class OptionStore: | |||
There was a problem hiding this comment.
Why not derive this class directly from dict ? That would simplify it a little bit, and remove one indirection.
There was a problem hiding this comment.
The entire point of the class is that it has only the methods explicitly written. That is, it does not have an indexing operation so users are forced to use the semantically meaningful named methods. At the moment they are simple direct accesses but in the future (actually already in the present in the optionrefactor branch) they do more like do per-subproject overrides transparently in one centralised place.
This also allows you to add breakpoints to the accessor methods which is exremely useful.
jpakkane
force-pushed
the
optstorerefactor
branch
4 times, most recently
from
fd83439
to
b7d5109
Compare
jpakkane
force-pushed
the
optstorerefactor
branch
from
b7d5109
to
f95f086
Compare
|
And now, finally, all green! |
mesonbuild/options.py
Outdated
| key = self.ensure_key(key) | ||
| return self.d[key].set_value(new_value) | ||
|
|
||
| # FIXME, this should be removed. |
There was a problem hiding this comment.
Yes. There should be separate methods for A) creating a new option with the given option and B) changing the type of an existing option. Due to reasons I can't remember I needed this but which will go away after some more refactoring.
In the end it might just need renaming, not removing so updating.
| @@ -182,7 +182,7 @@ def _add_languages(self, raw_langs: T.List[TYPE_var], required: bool, for_machin | |||
| if self.subproject: | |||
| options = {} | |||
| for k in comp.get_options(): | |||
| v = copy.copy(self.coredata.options[k]) | |||
| v = copy.copy(self.coredata.optstore.get_value_object(k)) | |||
There was a problem hiding this comment.
I see this pattern 3 times. Why not to have a get_value_object_copy() method?
There was a problem hiding this comment.
Because these will all go away. They will be replaced with named methods that have a proper semantic name. If any copying is to be done it should happen inside the OptionStore object.
| @@ -656,8 +656,8 @@ def generate(self, capture: bool = False, vslite_ctx: T.Optional[T.Dict] = None) | |||
| self.generate_dist() | |||
| mlog.log_timestamp("Dist generated") | |||
| key = OptionKey('b_coverage') | |||
| if (key in self.environment.coredata.options and | |||
| self.environment.coredata.options[key].value): | |||
| if (key in self.environment.coredata.optstore and | |||
There was a problem hiding this comment.
Maybe a has_value() -> bool method?
mesonbuild/backend/ninjabackend.py
Outdated
| @@ -3597,7 +3597,7 @@ def generate_gcov_clean(self) -> None: | |||
|
|
|||
| def get_user_option_args(self): | |||
| cmds = [] | |||
| for (k, v) in self.environment.coredata.options.items(): | |||
| for (k, v) in self.environment.coredata.optstore.items(): | |||
There was a problem hiding this comment.
| for (k, v) in self.environment.coredata.optstore.items(): | |
| for k, v in self.environment.coredata.optstore.items(): |
jpakkane
force-pushed
the
optstorerefactor
branch
from
f95f086
to
181c349
Compare
Does not work yet, but there are not that many failing tests any more. Posted for early review comments.