allow to directly import constants from easybuild.framework.easyconfig.constants #4144
Conversation
This allows e.g. `from easybuild.framework.easyconfig.constants import SYSTEM` instead of `easyconfig.constants.EASYCONFIG_CONSTANTS['SYSTEM'][0]`
Flamefire
force-pushed
the
ec-constants
branch
from
a447531 to
c799364
Compare
|
Failures in |
Good point, done in #4149 |
|
|
||
| # Add EasyConfig constants to export list | ||
| globals().update({name: value for name, (value, _) in EASYCONFIG_CONSTANTS.items()}) | ||
| __all__ = ['EXTERNAL_MODULE_MARKER', 'EASYCONFIG_CONSTANTS'] + list(EASYCONFIG_CONSTANTS.keys()) |
There was a problem hiding this comment.
Hmm, does __all__.extend(list(EASYCONFIG_CONSTANTS.keys()) work too?
Just to make sure we don't overlooked adding stuff to all if additional (real) constants are added to constants.py
There was a problem hiding this comment.
I tested that and no. __all__ has a default value if it is not set (only), so at that point it is an undefined variable.
It actually isn't really required because (almost) nobody does from easyconfig.constants import * which is the only use case of __all__, everything else works without it.
test/framework/easyconfig.py
Outdated
| """Test importing constants works""" | ||
| # Sanity check that importing an EC constant works as-if using EASYCONFIG_CONSTANTS | ||
| from easybuild.framework.easyconfig.constants import SYSTEM | ||
| self.assertEqual(SYSTEM, easyconfig.constants.EASYCONFIG_CONSTANTS['SYSTEM'][0]) |
There was a problem hiding this comment.
For SYSTEM, I would make this a hard check, rather than relying on EASYCONFIG_CONSTANTS:
self.assertEqual(SYSTEM, {'name': 'system', 'version': 'system'})There was a problem hiding this comment.
But that is not what is tested here. Only importing the constants should be tested here and that means that doing such an explicit import is the same as accessing the EASYCONFIG_CONSTANTS list, see the comment.
There was a problem hiding this comment.
I'm not saying you should not do the explicit import, you should.
I'm saying you should compare the value of SYSTEM directly to a hardcoded dict here, rather than relying on EASYCONFIG_CONSTANTS, otherwise the tests would still pass when the value for SYSTEM in EASYCONFIG_CONSTANTS in changed in the code.
There was a problem hiding this comment.
otherwise the tests would still pass when the value for
SYSTEMinEASYCONFIG_CONSTANTSin changed in the code.
Erm, This is exactly what you want, don't you?
- The value of
SYSTEMin ECs is that ofEASYCONFIG_CONSTANTS - Previously you would need to use
easyconfig.constants.EASYCONFIG_CONSTANTS['SYSTEM'][0]in hooks etc. - With this PR you can instead use
from easybuild.framework.easyconfig.constants import SYSTEMto the same effect - Hence the test checks that the imported value matches the value in
EASYCONFIG_CONSTANTSto verify it is indeed the same effect and has a matching comment
So I don't understand why the test shouldn't pass when easybuild.framework.easyconfig.constants.SYSTEM == easyconfig.constants.EASYCONFIG_CONSTANTS['SYSTEM'][0]. Could you explain your thought here?
Additionally I really dislike using hardcoded values as this results in unrelated tests failing. This was e.g. the case where a spelling fix to a doc caused another test to fail: #3219 -> #4158 So if some previously run other test for any reason changed the EASYCONFIG_CONSTANTS (which shouldn't happen, but I guess is what you had in mind) this test still passing is IMO actually a good thing.
There was a problem hiding this comment.
I consider a change to the value of a constant a breaking change, so tests should fail because of it.
Although no code should be relying on the value of the constant, it's not unlikely that some stuff does, and hence the value of the constant should never be changed.
Hardcoding the expected value in the tests is a way of catching that.
There was a problem hiding this comment.
I consider a change to the value of a constant a breaking change, so tests should fail because of it.
Ok understood. But I wouldn't do that here as that is not the purpose of the test: "importing an EC constant works as-if using EASYCONFIG_CONSTANTS".
I split the test into 2 where the 2nd ensures exactly that and the first is more like what a user would do and uses the hard-coded value. I'm still not a fan of doing that for the mentioned reason and would rather have dedicated tests to catch constant changes that shouldn't change or just say that the purpose of the constant is that the value does not matter so anything relying on it is a bug anyway.
|
Can we get this into the next release? It only adds stuff, so nothing should be at risk breaking and it's IMO a nice addition especially for hook-authors. |
We missed the window for EasyBuild v4.7.0 (which got delayed significantly already, so I wasn't keen on picking up extra stuff that was not entirely ready to merge yet), but getting this in for EasyBuild v4.7.1 should definitely work... Just take a look at the suggestion for the tests, and then it's good to go imho. |
boegel
changed the title
This allows e.g.
from easybuild.framework.easyconfig.constants import SYSTEMinstead ofeasyconfig.constants.EASYCONFIG_CONSTANTS['SYSTEM'][0]