Closed BapRx closed 1 year ago
Hey, very cool, thank you for your contribution!
It would be nice to advertise the new functionality a bit. Maybe add a short sentence in the "Generate your own configuration" section in docs/src/local_configuration.md
. Maybe even with a small example.
A test would be good. Take a look at e2e_tests/test_repos_find.py
, especially the test_repos_find()
test function. To me, there are three test cases that should be covered:
So I'd do the following:
test_repos_find()
that has three values: None
, "match"
, "nomatch"
.
None
simply does nothing.match
adds --exclude repo1
and checks that only repo2 is in the outputnomatch
adds --exclude foobar
and checks that both repo1
and repo2
are in the output (i.e. no change to the output check).Honestly, the e2e test code is very ... special, and hard to get into. If you finish the rust side, I'm happy to do the changes to the tests.
I'm open for suggestions to improve this
I'll take a closer look at the code itself and add comments directly if I find something that can be improved.
I'm not a rust dev
Now you are ;) Feel free to add yourself to the CONTRIBUTORS
file if you want.
Hey @hakoerber, thank you very much for taking the time to explain this. It's very interesting and I tried to follow your recommendations.
I also added an example in the doc, linted the pytests and added a few conditions in the test_repos_find()
function.
Absolutely awesome, thank you for this! Very cool that you also added e2e tests for this :+1:
There are just a few small things to fix before this can be merged. I already added some inline comments.
One thing that cannot be done inline: ruff
complains about unncecessary imports in e2e_tests/docker-rest/flask/app.py
. But these imports (github
and gitlab
) are actually necessary for the flask testing API to function correctly (and they have to be at the end of the file, isort
and ruff
are wrong here). A simple # noqa: E402,F401
at the end of those two imports should fix this.
All in all, as soon as just check
runs succssfully, I am more than happy to merge this!
just check
is happy :slightly_smiling_face: Thank you once again for the explanations, it's always very clear!
Yes, looks good! Merged.
Thank you for your contribution :)
If you want me to add you to the CONTRIBUTORS
file, let me know (also what I should put in there, like your github username or something else).
Thank you once again for the explanations, it's always very clear!
My pleasure.
Hi @hakoerber, I'm not a rust dev so there might be a better way to implement this (especially the case when no exclusion pattern is provided) but here's my PR to exclude paths from the
grm repos find local
discovery.I'm open for suggestions to improve this :+1: