563 add dependency info - #600
Conversation
|
I had to add yaml as dependency. |
|
Matthew Hambley (@MatthewHambley) , Sam Clarke-Green (@t00sa) - adding you, since I assume allynt is also on leave. |
Matthew Hambley (MatthewHambley)
left a comment
Matthew Hambley (MatthewHambley)
left a comment
There was a problem hiding this comment.
A few small issues around documentation and minor details.
There was a problem hiding this comment.
The documentation should probably mention the checkout-and-merge behaviour outlined in the docstring of RepoInfo.
There was a problem hiding this comment.
I've updated the documentation to mention this, but didn't extend the example code, instead added a link to SimSys_script repository (since that code is not that easy)
| to access and update them individually. Source and ref can | ||
| are None if there is no information for a repository. |
There was a problem hiding this comment.
"can are", looks like a change of mind mid sentence.
There was a problem hiding this comment.
Indeed, fixed.
| dep_info = DependencyInfo(yaml_file) | ||
|
|
||
| # Loop over all dependency repositories: | ||
| for repo in self.dependency_info.get_repo_names(): |
There was a problem hiding this comment.
Do we care about the keys here? Would it be tidier to just iterate over the repository information objects directly?
There was a problem hiding this comment.
The key is used in logging message. But I can indeed use a items() and avoid accessing the dict for getting the info object, which is more pythonic I think(?).
| 'flake8 >= 5.0.4', | ||
| 'mypy' | ||
| 'mypy', | ||
| 'pyyaml', |
There was a problem hiding this comment.
Is this not a permanent dependency. i.e. one for the dependencies = ... section above?
There was a problem hiding this comment.
I opted for dev, since not all Fab applications will need it - and if an application is using a yaml file, you would assume that they have yaml installed as part of their setup ;)
But, I don't mind, if you prefer to have it as a permanent dependency, I am happy to move this.
|
Ready for next review, |
2 participants
Adds DependencyInfo class (which is used in LFRic and UM) to Fab.