search

aiidateam / aiida-testing

A pytest plugin to simplify testing of AiiDA plugins.
MIT License
5 stars 6 forks source link

`mock-code`: Allow paths to be relative to the config file #52

Closed janssenhenning closed 2 years ago

janssenhenning commented 2 years ago

See #51

This PR allows paths in the .aiida-testing-config.yml file to be relative. These are interpreted to be relative to the config file location, since the location of the test execution can vary.

I followed the suggestion in the issue above and added the file path as a special key to the config, which is excluded when writing the config out. Another possibility would be to add the filepath as an attribute/property to the Config class. I don't have a preference for either way

greschd commented 2 years ago

Another possibility would be to add the filepath as an attribute/property to the Config class.

That seems like the cleaner solution to me, since it doesn't have the "usual" properties of a config entry (e.g., cannot be missing, or changed via the usual means).

greschd commented 2 years ago

Hey guys, so am I understanding correctly that you are proposing to specify the filepath of the .aiida-testing-config.yaml inside the .aiida-testing-config.yaml file?

No, AFAIU the goal is to provide the path at runtime in the Config (either as a direct attribute, or an entry in the dict), not hard-code it in the .yaml.

janssenhenning commented 2 years ago

Hey guys, so am I understanding correctly that you are proposing to specify the filepath of the .aiida-testing-config.yaml inside the .aiida-testing-config.yaml file?

The filepath is never written out to the config file. In the current implementation it is added as a special key to the instance of the Config class in from_file and excluded again in to_file.

I will switch the file path to be a property of the Config class instead of a special key to make this distinction clearer.

ltalirz commented 2 years ago

I will switch the file path to be a property of the Config class instead of a special key to make this distinction clearer.

Thanks, I guess that's what confused me (besides being a bit tired ;-) ).

Sounds all fine; happy to merge once it's done

janssenhenning commented 2 years ago

@ltalirz The file path is now implemented as a property The tests and docs build fail on my fork but I think this is related to aiida-core 2.0 (reentry is now not available)/and a newer sphinx version complaining about the configuration

ltalirz commented 2 years ago

Thanks @janssenhenning ! Let me have a look whether I can fix the CI

Are you using the package with AiiDA 1.6 or with AiiDA 2?

ltalirz commented 2 years ago

I've fixed CI. Could you please update this branch? Somehow I don't have the rights to do so

image
janssenhenning commented 2 years ago

I've fixed CI. Could you please update this branch? Somehow I don't have the rights to do so

Done 👍 It's probably because I used a fork in our Institute organization, where the rights are probably different

ltalirz commented 2 years ago

Thanks @janssenhenning !

Please note that one part of fixing CI was to make the limitation in the aiida-core version actually take effect, meaning it will force-install aiida<2 now.

However, I believe the package works with aiida2 as well - if you need that, let me know (or open a PR that removes the requirement and removes the reentry calls in the CI tests).