Add pre-commit and extended CI testing#13
Conversation
Update strategies with doc strings and use TYPE_CHECKING
Add `dev` extra with development requirements. Use Python 3.9 as minimum version.
Add checks for `project_slug` and `package_name` to pre_gen_hooks. Use organization value in email for default. Ensure the proper package name directory is used in `setup.py` - should the package_name and project_slug be very different.
Add .gitignore to generated repo. Update CI to properly run pytest in the generated repo and use a config file for cookiecutter instead of passing arguments through the CLI.
This script initializes the generated repository as a git repo. It furthermore adds an initial commit that includes all the generated files, with SINTEF as the author via the Team4.0 email. Finally, it sets up the `origin` remote using the supplied `git_url` input parameter and changes the branch name to `main`.
Check list of `git config` to assert whether or not git has been configured.
|
The pre gen hook that checks the |
|
To me this looks good. |
Cheers. If you leave an "approving" review, I've set up the PR to be auto-merged. |
@kriwiik should have the final word for this PR, that is why I left it open. |
For project_slug and package_name. But print a warning for both.
| print(read_output) | ||
|
|
||
| return {} | ||
| read_output = download_strategy.get(session) |
There was a problem hiding this comment.
This might be a stupid question, but is it necessary to use the session here? Looking at the get() functions of the download strategies in oteapi-core, I can't see that any of them use the session for anything.
There was a problem hiding this comment.
The session is just passed on to the download strategy in case it needs it. The session may e.g. be used to pass configurations from a filter's initialise() to a download strategy's initialise(). Not sure about the exact use case for passing the session to get(), but I think there might be similar cases for that.
There was a problem hiding this comment.
Indeed. All strategies have the get() and initialize() methods. The get() method being the "let's do this" method...
They all take in the session dictionary, which is specific to each call and is utilized to its full intent through otelib and the pipeline system. If you are familiar with "context"-concepts in programming this is essentially that. It's a set up variables that is shared and passed along as the "context" for a single "run" through the uses of a set of strategies :)
TL;DR: Yes, the session is necessary to ensure the "context" is available for the download strategy.
There was a problem hiding this comment.
And for them not using the session, this will differ wildly from strategy to strategy.
I like the idea of some standard practices and usages of session however that we can think about and implement - like ensuring the local variables are always updated according to the values in the session - before falling back on default values, always check the session for that given value - etc.
There was a problem hiding this comment.
They all take in the session dictionary, which is specific to each call and is utilized to its full intent through otelib and the pipeline system. If you are familiar with "context"-concepts in programming this is essentially that. It's a set up variables that is shared and passed along as the "context" for a single "run" through the uses of a set of strategies :)
I am not familiar with "context"-concepts in programming, but I will try to read up on it + look closer at otelib to try and make sense of this.. I think I understand the point of passing the session along, but don't understand how that really happens when the functions don't use or return the session. But oh well, the PR looks good to me now and I will approve it! :)
There was a problem hiding this comment.
Let's chat about it over a rundown of otelib at some point :)
4 participants
Closes #12.
Update strategies with doc strings and use
typing.TYPE_CHECKING.Add README and a bit of documentation information.