Checking that all files have owners. #441
Conversation
tests/tooling/test_codeowners.py
Outdated
| 'keras_contrib/optimizers/padam.py', | ||
| 'keras_contrib/optimizers/yogi.py', | ||
| 'keras_contrib/metrics/crf_accuracies.py', | ||
|
|
| 'keras_contrib/regularizers', | ||
| 'keras_contrib/wrappers' | ||
| ] | ||
| directories_to_test = [path_to_keras_contrib / x for x in directories_to_test] |
There was a problem hiding this comment.
Why the use of pathlib instead of classic os.getcwd and os.path.join ?
There was a problem hiding this comment.
Less typing?
There was a problem hiding this comment.
This test only runs on python 3 on travis btw, so it should be ok to use pathlib.
tests/tooling/test_codeowners.py
Outdated
| files_with_owners = [x[0] for x in parse_codeowners()] | ||
| for children in directory.iterdir(): | ||
| if children.is_file(): | ||
| if children.name in pattern_exclude: |
There was a problem hiding this comment.
combiner les deux en un seul if (children in exclude) or (children.name in pattern_exclude)
|
We have an owner for the applications. See #415 (comment) |
CONTRIBUTING.md
Outdated
|
|
||
| ## About the `CODEOWNERS` file | ||
|
|
||
| If you add a new feature to keras-contrib, you should add yourself and your file in the `CODEOWNERS` file. Doing so will, in the future, tag you whenever an issue or a pull request about your feature is opened. Be aware that it represents some work, and in addition of being tagged, you should review new pull requests related to your feature. |
There was a problem hiding this comment.
you should review new pull requests related to your feature.
Will it be possible to soften the tone a lil bit here? Something like "We would appreciate X" instead of "You should do X".
There was a problem hiding this comment.
Sure thing, I'll change it.
|
If there is no objection, I'll merge this on Tuesday. |
- What I did
I added a test to check that all files have owners.
- How I did it
Read the files in a selected list of directories, then check that those files are either explicitly excluded (temporary, before v1.0) or in the CODEOWNERS.
- How you can verify it
The build passes
this implements part of #418