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

Rework ReviewBot.CommandLineInterface to provide class option. #622

Merged

Conversation

@jberry-suse
Copy link
Contributor

jberry-suse commented Dec 28, 2016

Alleviates the need for a lot of duplicate code, some of which is already out-of-sync. Some examples of out-of-sync code are as follows.

  • check_source_in_factory.py: not include group parameter
  • check_tags_in_requests.py: missing default user

Either way this is primarily intended as a cleanup. I ran into this when I started porting osc-check_source.py to ReviewBot and needed to copy/paste a bunch of code for no good reason.

Alleviates the need for a lot of duplicate code, some of which is already
out-of-sync.
@jberry-suse jberry-suse force-pushed the jberry-suse:ReviewBot.CommandLineInterface-class branch from b69dcc6 to a7b3f01 Dec 28, 2016
@coveralls

This comment has been minimized.

Copy link

coveralls commented Dec 28, 2016

Coverage Status

Coverage remained the same at 44.331% when pulling a7b3f01 on jberry-suse:ReviewBot.CommandLineInterface-class into cc7888e on openSUSE:master.

@coolo coolo merged commit 7431af4 into openSUSE:master Jan 2, 2017
1 check passed
1 check passed
continuous-integration/travis-ci/pr The Travis CI build passed
Details
@lnussel

This comment has been minimized.

Copy link
Member

lnussel commented Jan 2, 2017

So much better. Thanks! :)
With this the ReviewBot.setup_checker bits could be moved to postoptparse so bots override postoptparse instead. Might be more straight forward I guess.
Would it make sense to pass the class to CommandLineInterface? Save another line in very simple bots.

@jberry-suse jberry-suse deleted the jberry-suse:ReviewBot.CommandLineInterface-class branch Jan 3, 2017
@jberry-suse

This comment has been minimized.

Copy link
Contributor Author

jberry-suse commented Jan 3, 2017

When making the changes I was thinking postoptparse() seemed the like the intended place, but imagine that given setup_checker() had to be overridden it was simply avoided to add the other method. If changed the super().postoptparse() would still need to be called and self.checker referenced instead of bot returned from super().setup_checker(). I am happy to make the change although I am not sure much is gained.

The downside of passing the class to the constructor is that it would have to be removed before passing along to cmdln.Cmdln.__init__() which ends up being a tad ugly for what is gained.

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

Successfully merging this pull request may close these issues.

None yet

4 participants
You can’t perform that action at this time.