#2395 Optimize the generation of the gitolite configuration
Merged by pingou. Opened by pingou.
optimize_gitolite_conf  into  master

Download 2395.patch

With this change we support only updating part of the configuration
instead of re-generating it entirely all the time.

Signed-off-by: Pierre-Yves Chibon pingou@pingoured.fr

rebased

Jenkins seems to be unable to git clone, so I guess we can ignore it.

I recommend writing a docblock for this function describing its behaviour and the project argument (and its type). I see that there's a test for project being -1 - that would be good to document here too.

Nice docblock! Can you also mention the type of project here? I assume it's a pagure.something.something.Project? ☺

It would be good to document this method's arguments and their types.

This method is a bit long - I recommend breaking into some smaller helper methods to make it simpler and easier to test.

It would also be good to document the return value and it's type.

I've seen this list of types of repos in another PR before - a nice refactor might be to make this list a "constant" somewhere in Pagure so all the places that need it can just import that constant and use it rather than redefining it.

This code feels familiar. Did I review another PR that was similar to this recently? Is there an opportunity for some code sharing to happen?

It would be good to document the parameters, the return value, and the types of both here.

It's being refactor into a smaller function ;-)

This is alllmost the same list, but I guess it has '' instead of repos. It might or might not make sense to reuse a modified copy of a constant here too like I suggested above.

I used to be even longer! :D

This one is a little long too - it might be nice to break it into helper methods too.

It would be nice to document the new project parameter here.

Oh maybe this is why that code looked so familiar. It looks like you just moved it? If so, disregard my earlier comment ☺

It would be good to document the new project parameter and it's type.

While you are in here, it would be nice to document the uuid parameter and its type.

I recommend documenting the new parameters here.

It could be nice to document the parameters and return value here ☺

Here too.

For PEP-8, you could drop this newline.

For PEP-8, you could add a newline here.

I recommend dropping this print comment.

Do you need this with block?

And this one?

To ensure the file is present.

If the project is None but the file does not exist, the entire config will be re-generated.

cf https://pagure.io/pagure/pull-request/2395#_9,112

I recommend dropping this line.

And this line.

Here as well, to have a file present (doesn't matter if it is empty or not)

Rather that re-running the other test, you could take what you need from it and make it a commonly-used helper method.

I kinda like keeping these, they are most helpful when needing to debug the tests :)

All comments are optional, LGTM!

I need the file it generates so I figure it was just as easy to run the entire test

Ah - interestingly I am surprised to learn that Python didn't have an equivalent to touch until 3.4: https://docs.python.org/3/library/pathlib.html#pathlib.Path.touch

So I guess that doesn't help you since you are on 2.7, but maybe one day ☺

Going to adjust the docstrings and merge :)

rebased

Adjusted, thanks for the review! :)

Pull-Request has been merged by pingou

Metadata