Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

add 'hide-invalid-repositories' option #266

Open
wants to merge 10 commits into
base: master
Choose a base branch
from

Conversation

rotsix
Copy link
Contributor

@rotsix rotsix commented Apr 22, 2021

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
Copy link
Contributor Author

rotsix commented Oct 12, 2021

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

@jonashaag
Copy link
Owner

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
Copy link
Owner

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.

@rotsix
Copy link
Contributor Author

rotsix commented Oct 12, 2021

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

@jonashaag
Copy link
Owner

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.

@rotsix
Copy link
Contributor Author

rotsix commented Oct 13, 2021

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 :)

klaus/views.py Outdated Show resolved Hide resolved
@jonashaag
Copy link
Owner

jonashaag commented Oct 13, 2021

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?

@rotsix
Copy link
Contributor Author

rotsix commented Oct 13, 2021

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.

@jonashaag
Copy link
Owner

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

@rotsix
Copy link
Contributor Author

rotsix commented Oct 13, 2021

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
Copy link
Contributor Author

rotsix commented Oct 13, 2021

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

@jonashaag
Copy link
Owner

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

@jonashaag
Copy link
Owner

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

@rotsix
Copy link
Contributor Author

rotsix commented Oct 14, 2021

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.

@jonashaag
Copy link
Owner

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

@rotsix
Copy link
Contributor Author

rotsix commented Oct 14, 2021

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.

@jonashaag
Copy link
Owner

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

@rotsix
Copy link
Contributor Author

rotsix commented Oct 15, 2021

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
Copy link
Contributor Author

rotsix commented Nov 21, 2021

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

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

None yet

2 participants