klaus icon indicating copy to clipboard operation
klaus copied to clipboard

add 'hide-invalid-repositories' option

Open rotsix opened this issue 4 years ago • 19 comments

Hey. As I don't mind if Klaus find some invalid repositories, I added an option to hide those.

During the app building, when it formats the {in,}valid repositories, it checks for this newly created option and dismiss the invalid ones.

I hope that's enough for you! Thanks for this super software :)

rotsix avatar Apr 22 '21 21:04 rotsix

Hey @jonashaag, could you please consider merging this PR or giving your opnion on it? Many thanks!

rotsix avatar Oct 12 '21 20:10 rotsix

I’m sorry, I think I didn’t respond here because the tests fail and I wanted to wait for you to fix them before reviewing the PR

jonashaag avatar Oct 12 '21 20:10 jonashaag

Review feedback: please add the new option at the end of the keyword arguments. Also I think it’s more obvious to only look at the hide flag where the invalid repos are rendered, not where the list of invalid repos is constructed.

jonashaag avatar Oct 12 '21 20:10 jonashaag

Hey again, thank you for your time. I believe all is working this time!

rotsix avatar Oct 12 '21 22:10 rotsix

Thanks a lot, please move the new setting to very end so as to make the changes backwards compatible.

Also what do you think about adding a "N invalid repositories hidden" to the repo list page? Do you think that could be helpful? Or maybe we can just log a warning to the terminal. Just a way for people to discover that some stuff has been hidden. Not sure what's the best option here.

jonashaag avatar Oct 13 '21 06:10 jonashaag

move the new setting to very end

I could move it at the very end but the Klaus constructor takes ctags_policy as a keyword argument while it takes the others as positional. Making it the last makes the new option a keyword argument. I guess that's something you want.

Just a way for people to discover that some stuff has been hidden

Yes some stuff has been hidden but not by default, that's the user's will. I agree that both a normal logging as well as a small message to the repo list page could be useful. I'm on it :)

rotsix avatar Oct 13 '21 13:10 rotsix

I just tried this, but when I start klaus --hide-invalid-repos some_invalid_repo, I still get klaus: error: argument DIR: 'some_invalid_repo': Not a Git repository. I was expecting the option to ignore that repo, or did I misunderstand what the option is supposed to do?

jonashaag avatar Oct 13 '21 15:10 jonashaag

The added option won't change this behavior. You can't pass an invalid Git repository to klaus. This option is useful when you pass a folder containing directories, then the invalid ones aren't displayed.

rotsix avatar Oct 13 '21 15:10 rotsix

But how do you pass a folder on the command line?

jonashaag avatar Oct 13 '21 20:10 jonashaag

Using the standalone application (python bin/klaus) won't load an invalid repository and end with an error (klaus: error: argument DIR: <path>: Not a Git repository).

By wrapping Klaus within uwsgi, I believe you can make it works with some invalid repositories. That's how I use it daily with the Docker image.

rotsix avatar Oct 13 '21 20:10 rotsix

I just investigated and found what causes that. When running the standalone Klaus, the argument parser tries to match the passed directories with the git_repository function (./bin/klaus). An invalid repository will not match, raise an exception and cause the end of the run.

When wrapping using uwsgi (by example), it by-pass the initial argument type-check and sort the repositories between valid and invalid.

One way around is to disable this git-type check in the argument parser but that's not the goal of this PR. :)

EDIT: type check

rotsix avatar Oct 13 '21 20:10 rotsix

But then the command line flag does not make a difference, does it?

jonashaag avatar Oct 14 '21 07:10 jonashaag

I think it would be fine to have it disable the type check.

jonashaag avatar Oct 14 '21 07:10 jonashaag

This commit disables the type check.

The command line flag doesn't make a difference with this previous type check as the repositories couldn't go "through" the type checker. Now it is disabled you can pass invalid repositories that will (or will not using this new option) get rendered.

rotsix avatar Oct 14 '21 11:10 rotsix

Sorry, maybe wasn't clear enough – we can change behaviour only when --hide-invalid-repos is given, otherwise we're breaking backwards compat.

jonashaag avatar Oct 14 '21 11:10 jonashaag

I don't think we can change behavior only when the option is given, that's all or nothing as the type check is done by the argument parser. If we really want to keep the Git type check, either we do it later or we keep this option as being useless when using the Python executable directly.

rotsix avatar Oct 14 '21 12:10 rotsix

Yes, let's do the check later, for example in main where we do the other checks.

jonashaag avatar Oct 14 '21 16:10 jonashaag

We could move the check in main but this can lead to 2 situations:

  • nothing changes, the type check is done in main and the program still stops if an invalid repository is given (actual behavior)
  • the type check is done in every case (not only when running via klaus binary directly), this would break backward compatibility by crashing the program when using uwsgi

To be honest, I can't really understand the type check and why it is done. On the repo list page, the invalid repositories are even displayed when using uwsgi, so why doing this test?

rotsix avatar Oct 15 '21 10:10 rotsix

Hey @jonashaag, have you had some time to think about those type checks? :)

rotsix avatar Nov 21 '21 21:11 rotsix