Added variable existence check

[ljoakim]

Closes #8

Opening a PR to get feedback on base check: check_variable_existance(...). I'm keeping it as draft for now.

• check_variable_existance is implemented
• I've added a check_id argument to the function, to allow for adding the check identifier from the qc-table to the test name string.
• I've set up a test, which tests the two basic outcomes of the check. Comments:
  • I added a new checks subfolder to the tests folder, to follow the structure of the compliance_checker/ folder.
  • In the new conftest.py file, there is a fixture that creates a temporary nc-file used with the test.

Please provide feedback.

[IlianaGhazali-Meteo]

I don't know how the IPSL is planning to organize the project. Personally, I have followed the git conventional rules to name my commit (feat(scope): short description #id-issue) and create my feature branch. Let me know if you want to do things differently.

[IlianaGhazali-Meteo]

Maybe we can add the check_id optionally on the TestCtx class, otherwise it will be lost. To discuss with the team.

[ljoakim]

It's a good suggestion. The only caveat I can think of is that this requires modifications in the all-shared code in base.py. The interface should be not be a problem with a default valued argument though (check_id=None). Also, that would enable using a single implementation of any type of additional formatting, like where I put check_id in brackets.

[ljoakim]

I don't know how the IPSL is planning to organize the project. Personally, I have followed the git conventional rules to name my commit (feat(scope): short description #id-issue) and create my feature branch. Let me know if you want to do things differently.

I like it, and I don't have any better ideas. In my experience such rules are hard to uphold when there are many contributors, and it has honestly sort-of left my daily practice :) But I think it is a good idea, and it should be clearly stated in some kind of contributor guidelines. Can it be enforced with an issue template on github?

[ljoakim]

I'm including you @Ayoubnac1 in the discussion. There are a few open questions regarding structure/design still that need to be discussed (e.g. the threads above), and I/we need input from others.

[ljoakim]

Thanks for the comments @IlianaGhazali-Meteo @Ayoubnac1

I resolved most of the comments myself, to put focus on the remaining topics. Please do unresolve or add new comments if there is anything additional you'd like discussed.

I've added __init__.py files to the checks and variable_checks folders, which effectively replaces the .gitkeep files.

To me there are a few things left to discuss:

1. The still unresolved check_id discussion above.
2. A short discussion on how to name feature branches, issues and pull-requests, as mentioned in one of the comments above.
3. Tests: I will focus a bit on the already existing tests to get a sense of the class-style approach. It's not the style I'm used to, but I agree that it is good to keep the style similar to existing tests. In internal projects we usually categorise the tests into unit and integration tests. For unit tests, which I think we are aiming to implement at this point, we try to mirror the folder structure of the tested package, which makes it easy to find, add and modify specific tests. Do you have input on any of this?

[IlianaGhazali-Meteo]

I don't know how the IPSL is planning to organize the project. Personally, I have followed the git conventional rules to name my commit (feat(scope): short description #id-issue) and create my feature branch. Let me know if you want to do things differently.

I like it, and I don't have any better ideas. In my experience such rules are hard to uphold when there are many contributors, and it has honestly sort-of left my daily practice :) But I think it is a good idea, and it should be clearly stated in some kind of contributor guidelines. Can it be enforced with an issue template on github?

yes, there are several strategies for enforcing the use of a git convention, whether on the local or server side. It depends on what is decided on the project.

[IlianaGhazali-Meteo]

Thanks for the comments @IlianaGhazali-Meteo @Ayoubnac1

I resolved most of the comments myself, to put focus on the remaining topics. Please do unresolve or add new comments if there is anything additional you'd like discussed.

I've added __init__.py files to the checks and variable_checks folders, which effectively replaces the .gitkeep files.

To me there are a few things left to discuss:

1. The still unresolved check_id discussion above.
2. A short discussion on how to name feature branches, issues and pull-requests, as mentioned in one of the comments above.
3. Tests: I will focus a bit on the already existing tests to get a sense of the class-style approach. It's not the style I'm used to, but I agree that it is good to keep the style similar to existing tests. In internal projects we usually categorise the tests into unit and integration tests. For unit tests, which I think we are aiming to implement at this point, we try to mirror the folder structure of the tested package, which makes it easy to find, add and modify specific tests. Do you have input on any of this?

I also think we can remove .gitkeep files, even if the use of init.py is not equivalent and optional, we can use these files as already done on the project.
I don't have more comments on the points you listed, it depends on what we want to implement on the project, several options are possible @Ayoubnac1.

[Edit] :
finally, I'd insist on point 2 after seeing the git history of the develop branch, which isn't very readable and clear. We really need features branches before pushing directly to develop with too many “WIP” commits. In fact, the develop branch should be protected and direct commits prohibited in a nominal workflow.
This will also allow us to comment before integration.
For example, @Ayoubnac1 the add_pass method has no arguments. The ioos project specifies not to add success messages in TestCtx. Only error messages have to be added. Otherwise, we'd need a logging system to trace events.

[ljoakim]

I've made some minor changes to the tests, adding comments and moving the new conftest.py file to the variable_checks folder to separate creation of datasets for specific check categories. I looked into the existing tests a bit, inheriting the BaseTestCase class, and I think it is probably useful for later 'higher level' integration tests. For the unit tests I prefer having control of the details wrapped in its methods, and it's nice to keep context focused with any dependencies/fixtures close to reduce dependency on shared resources (and code). It's all up for discussion, but I'm fine with how the new test is implemented now.

finally, I'd insist on point 2 after seeing the git history of the develop branch, which isn't very readable and clear. We really need features branches before pushing directly to develop with too many “WIP” commits. In fact, the develop branch should be protected and direct commits prohibited in a nominal workflow.

I second this 👍