Deprecate repo_type parameter in get_repo_open and get_repo_initialise #321
Conversation
This commit deprecates `repo_type` parameter in `get_repo_open` and `get_repo_initialise` functions. As we discussed in PR#318 [1], these should be deprecated before 4.0.0 release. [1] mtreinish#318
There was a problem hiding this comment.
Thanks for doing this, sorry for the slow turnaround on reviewing this. For the most part it looks good to me, but I think there is a find/replace typo issue on all the updated calls. Other than that I had a question inline about writing to stderr in addition to emitting a warning.
stestr/commands/failing.py
Outdated
| @@ -103,7 +103,7 @@ def failing(repo_url=None, list_tests=False, subunit=False, | |||
| for failures. | |||
| :rtype: int | |||
| """ | |||
| repo = util.get_repo_open('file', repo_url) | |||
| repo = util.get_repo_open(repo_url) | |||
There was a problem hiding this comment.
I'm curious does this work as expected? Aren't we passing repo_url here as the value for the repo_type argument. I think we're probably not catching this in tests because we use the default None and nothing is triggering the deprecation warning. I think this should be:
| repo = util.get_repo_open(repo_url) | |
| repo = util.get_repo_open(repo_url=repo_url) |
There was a problem hiding this comment.
Thanks, yeah, my bad.
stestr/commands/history.py
Outdated
| @@ -228,7 +228,7 @@ def history_list(repo_url=None, show_metadata=False, | |||
| else: | |||
| field_names = ('Run ID', 'Passed', 'Runtime', 'Date') | |||
| try: | |||
| repo = util.get_repo_open('file', repo_url) | |||
| repo = util.get_repo_open(repo_url) | |||
There was a problem hiding this comment.
| repo = util.get_repo_open(repo_url) | |
| repo = util.get_repo_open(repo_url=repo_url) |
stestr/commands/history.py
Outdated
| @@ -285,7 +285,7 @@ def history_show(run_id, repo_url=None, subunit_out=False, | |||
| :rtype: int | |||
| """ | |||
| try: | |||
| repo = util.get_repo_open('file', repo_url) | |||
| repo = util.get_repo_open(repo_url) | |||
There was a problem hiding this comment.
| repo = util.get_repo_open(repo_url) | |
| repo = util.get_repo_open(repo_url=repo_url) |
stestr/commands/history.py
Outdated
| @@ -352,7 +352,7 @@ def history_remove(run_id, repo_url=None, stdout=sys.stdout): | |||
| :rtype: int | |||
| """ | |||
| try: | |||
| repo = util.get_repo_open('file', repo_url) | |||
| repo = util.get_repo_open(repo_url) | |||
There was a problem hiding this comment.
| repo = util.get_repo_open(repo_url) | |
| repo = util.get_repo_open(repo_url=repo_url) |
stestr/commands/init.py
Outdated
| @@ -43,7 +43,7 @@ def init(repo_url=None, stdout=sys.stdout): | |||
| :rtype: int | |||
| """ | |||
| try: | |||
| util.get_repo_initialise('file', repo_url) | |||
| util.get_repo_initialise(repo_url) | |||
There was a problem hiding this comment.
| util.get_repo_initialise(repo_url) | |
| util.get_repo_initialise(repo_url=repo_url) |
stestr/commands/run.py
Outdated
| @@ -380,7 +380,7 @@ def run_command(config='.stestr.conf', | |||
| stdout.write(msg) | |||
| exit(1) | |||
| try: | |||
| repo = util.get_repo_initialise('file', repo_url) | |||
| repo = util.get_repo_initialise(repo_url) | |||
There was a problem hiding this comment.
| repo = util.get_repo_initialise(repo_url) | |
| repo = util.get_repo_initialise(repo_url=repo_url) |
stestr/commands/run.py
Outdated
| @@ -634,7 +634,7 @@ def run_tests(): | |||
| # the result from the repository because load() returns 0 | |||
| # always on subunit output | |||
| if subunit_out: | |||
| repo = util.get_repo_open('file', repo_url) | |||
| repo = util.get_repo_open(repo_url) | |||
There was a problem hiding this comment.
| repo = util.get_repo_open(repo_url) | |
| repo = util.get_repo_open(repo_url=repo_url) |
stestr/commands/slowest.py
Outdated
| @@ -79,7 +79,7 @@ def slowest(repo_url=None, show_all=False, | |||
| :rtype: int | |||
| """ | |||
|
|
|||
| repo = util.get_repo_open('file', repo_url) | |||
| repo = util.get_repo_open(repo_url) | |||
There was a problem hiding this comment.
| repo = util.get_repo_open(repo_url) | |
| repo = util.get_repo_open(repo_url=repo_url) |
stestr/config_file.py
Outdated
| @@ -164,7 +164,7 @@ def group_callback(test_id, regex=re.compile(group_regex)): | |||
| else: | |||
| group_callback = None | |||
| # Handle the results repository | |||
| repository = util.get_repo_open('file', repo_url) | |||
| repository = util.get_repo_open(repo_url) | |||
There was a problem hiding this comment.
| repository = util.get_repo_open(repo_url) | |
| repository = util.get_repo_open(repo_url=repo_url) |
stestr/repository/util.py
Outdated
| if repo_type is not None: | ||
| msg = ("WARNING: Specifying repository type is deprecated and will be " | ||
| "removed in future release.\n") | ||
| sys.stderr.write(msg) |
There was a problem hiding this comment.
I'm not sure we need to write the warning to stderr here. We were doing that for the CLI to make sure users saw the warning as the DeprecationWarning is normally not seen. But since the repo type argument no longer exists on the CLI this is only for python library interface users and I think relying on the warnings interface may be enough for them. What do you think?
There was a problem hiding this comment.
well, yeah, indeed. I'll remove this one and get_repo_initilise()'s one, too.
This commit removes warning messages for the stderr because these interfaces are only for python library interface not for CLI. We don't need to show them to users.
This commit fixes to use the updated call correctly.
|
@mtreinish Thank you very much for your review! I think it's ready for review now. Can you take a look at it again when you have the time? |
This commit deprecates
repo_typeparameter inget_repo_openandget_repo_initialisefunctions. As we discussed in PR#318 [1], theseshould be deprecated before 4.0.0 release.
[1] #318