POC Python profiles with django settings inspired syntax #8547
Conversation
|
|
||
| class PythonProfilesTest(unittest.TestCase): | ||
|
|
||
| def setUp(self): |
There was a problem hiding this comment.
Please start using modern pytest syntax with fixtures.
|
|
||
| options.shared = True | ||
|
|
||
| env.env_var = "bar" |
There was a problem hiding this comment.
Lets aim to use the Environment composition defined in #8534
There was a problem hiding this comment.
I have added a similar test with append, prepend, delete and so syntax to the env object
| self.client = TestClient() | ||
| self.pr1_content = textwrap.dedent(""" | ||
| import os | ||
| from conans import settings, options, env |
There was a problem hiding this comment.
This import of a global object, and the way the object is re-initialized for each profile doesn't look good.
It would be cleaner to do something like:
from conan.profiles import Settings, Options # To be define where it is, definitely in the new ``conan.`` scope, but not in the root
from conan.tools.env import Environment # the new one
settings = Settings()
settings.os = "Windows"
env = Environment()
env.append_path("PATH", os.getenv("MyLocalPathVar"))There was a problem hiding this comment.
I agree, the re-initilialization is not very elegant that way but wanted to try the clean syntax of not having to initialize a new object in the profile file.
I think it is easier to just have seettings initialized and you can apply the values without having to "compose" the "Settings()" object. You only load the files in the given order and just apply pure python rules.
There was a problem hiding this comment.
Not worth if we incur in such code anti-pattern, this access to global objects is bad, and we have also had very bad experience with it.
| @@ -10,6 +12,64 @@ | |||
| from conans.util.files import load | |||
|
|
|||
|
|
|||
| class ProfileOptions(defaultdict): | |||
There was a problem hiding this comment.
These classes cannot be here, this is too global of scope. Better move them to conan.profile
| return {key: item.value() for key, item in self._data.items()} | ||
|
|
||
|
|
||
| settings = ProfileSettings() |
There was a problem hiding this comment.
Remove these global objects, this is an anti-pattern.
|
|
||
| @staticmethod | ||
| def _init_profile(): | ||
| import conans |
There was a problem hiding this comment.
Continuation of the anti-pattern of the global objects, please avoid it.
|
|
||
| @pytest.fixture | ||
| def client(): | ||
| return TestClient() |
There was a problem hiding this comment.
A fixture of 1 line provides no value, they are for initialization reuse, but 1 liner is not worth (actually counter productive, losing locality is bad for reading tests). You can put all 3 fixtures in just 1, if you are always using them together: (client, pr1_content, pr2_content):. Also include the client.save() in the fixture.
|
Superseded by #9147 |
Coming from #7922
See django settings docs: https://docs.djangoproject.com/en/3.1/topics/settings/