Deprecate the partial flag #117
Conversation
mtreinish
force-pushed
the
deprecate-partial
branch
from
a32f46f
to
11de57e
Compare
|
ooh, nice a .006% coverage increase :p |
There was a problem hiding this comment.
LGTM, but I noticed one tiny thing about the help message.
And it seems like the DeprecationWarning is ignored by default[1]. So, I actually don't see the warnings.warn() message in my env. Is this your intention?
[1] https://docs.python.org/2/library/warnings.html#default-warning-filters
stestr/commands/run.py
Outdated
| help="DEPRECATED: Only some tests will be run. Implied" | ||
| " by --failing. This option is deprecated and no " | ||
| "longer does anything. It will be removed in the" | ||
| "future") |
There was a problem hiding this comment.
Do you want to add a white space before future?
There was a problem hiding this comment.
oh, good catch. I'll respin this in a bit
|
Yeah, I saw the same thing in the docs and was debating on that, I'm not sure which is better. This is a warning for deprecation, and I don't necessarily want to spam people with it. But I do want it to be discoverable which this isn't. What do you think? |
mtreinish
force-pushed
the
deprecate-partial
branch
from
5ece957
to
b531108
Compare
|
Heh, well I was asking your opinion there. I don't actually know what is best here, I just read the warning module docs and that seemed to be what we wanted, because this is a deprecation. But, it might not be best. Maybe it's better to just not set a warning type and use the default so it'll be printed. I don't feel strongly either way. Yeah, --partial shouldn't ever really be used it didn't really do anything so either way it's fine. It's more about signaling our users that the flag is going away so they'll need to update their usage. |
|
I actually expected the warnings would be printed. But, I think we need to write a bit more code to print them. I don't think it's worth doing that. |
|
Well I think it's the warning class there. On py3 I know they print regular warnings by default. I can switch it to a print too. Let me test some things locally and figure out the best compromise here |
mtreinish
force-pushed
the
deprecate-partial
branch
from
cae2b16
to
aa2059c
Compare
|
ok, I just removed the deprecation warning class type from the warn() call. In my local testing it prints once to stdout when you use the --partial flag. Granted it's not the most clean, but that's because upstream python never thinks about the end user experience. Do you think I should just switch it to a print? |
There was a problem hiding this comment.
Thanks for updating. LGTM.
I feel this is better than the original one from a user perspective. Or I was thinking to add a filter code like this.
warnings.simplefilter('default', category=DeprecationWarning)But I feel this is just a simple one line, but I think it probably causes some side effects.. So, my opinion is that just removing the class(treat as a UserWarning) is enough here.
The white space is a bit weird but I think it shouldn't be a blocker of this patch. We can fix it with a following patch easily.
stestr/commands/run.py
Outdated
| @@ -194,6 +199,9 @@ def run_command(config='.stestr.conf', repo_type='file', | |||
| for failures. | |||
| :rtype: int | |||
| """ | |||
| if partial: | |||
| warnings.warn('The partial flag is deprecated and has no effect ' | |||
There was a problem hiding this comment.
heh, there are two white spaces after the word 'effect'.
The partial flag is vestigial and left over from the original testr fork. It was originally used to indicate a run did not execute the entire test suite. However this doesn't really come into play anywhere else and it's not stored in the repository either. I think the eventual intent was to treat those records as incomplete for other purposes but that never really was implemented. With the introduction of load to run-id and also a run --combine this isn't really needed anymore either. It's not really something that should be user facing either. The user shouldn't have to tell the runner whether the run is the full suite or not. This commit removes the functional bits from the cli (and their python api commands) and deprecates all the flags for partial. The repository partial usage remains intact because it's coupled to how the file repository runs. (it uses a memory repository with partial=True before writing it to disk) So more care is needed before we rip that out, but in the mean time this removes and deprecates all the end user facing pieces. Closes issue #108
mtreinish
force-pushed
the
deprecate-partial
branch
from
aa2059c
to
dd11975
Compare
|
Ok, the latest rev quickly fixes the double space, I'll merge it after the CI jobs come back. |
The partial flag is vestigial and left over from the original testr fork.
It was originally used to indicate a run did not execute the entire test
suite. However this doesn't really come into play anywhere else and
it's not stored in the repository either. I think the eventual intent
was to treat those records as incomplete for other purposes but that
never really was implemented. With the introduction of load to run-id
and also a run --combine this isn't really needed anymore either. It's not
really something that should be user facing either. The user shouldn't
have to tell the runner whether the run is the full suite or not.
This commit removes the functional bits from the cli (and their python
api commands) and deprecates all the flags for partial. The repository
partial usage remains intact because it's coupled to how the file
repository runs. (it uses a memory repository with partial=True before
writing it to disk) So more care is needed before we rip that out, but
in the mean time this removes and deprecates all the end user facing
pieces.
Closes issue #108