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 (weakly required) --all argument to forget-results as well as adding --all-tests argument #18

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

Conversation

hlovdal
Copy link
Contributor

@hlovdal hlovdal commented Mar 14, 2021

As mentioned in issue #11, the forget-results command's default behaviour of deleting everything will be super dangerous if it ever gains support for deleting individual results. I have now added a --all option to that command which gives a warning if it is not given but otherwise the command behaves as before (except it now prints status a line on success).

In addition a bonus --all-tests option is also added that will as the name implies delete results for all the defined tests. So regardless of if the command ever gets support for deleting individual results, it now makes sense to be required to explicitly specify which tests that should be forgotten.

I have no plans on working on adding support for deleting individual test results with the forget-results command, but by adding --all now this should be less of a problem to do some time in the future.


When refactoring and splitting functions I let the code stay in the same place as before as much as possible to make the diffs small, but that might have resulted in a bit untidiness with regards to code layout in the file. Maybe you want to move around on some of the functions.

I have kept the colouring of the warning consistent with the existing code, but I really think it deserves a more prominent colour.

@mhagger
Copy link
Owner

mhagger commented Mar 24, 2021

I admit that cmd_list() was a kludge, but instead of turning its reading part into a separate function, I think it would be better to build a more robust way of reading tests. In particular, I don't want to lock us into thinking of a Test as just a command, because I think there will be value from adding other types of test as well (for example, I'd love to add a new type of test that causes multiple other tests to be run).

I've implemented what I think the reader abstraction could look like in #19. Also, I implemented this as an iterator, because I think that makes it easier to use. What do you think of it? If it's OK with you, I'd rather have the functionality of this PR rewritten based on iter_tests() rather than your process_all_tests_one_by_one() function.

@hlovdal hlovdal force-pushed the forget-all-argument branch from 978b489 to 5ffef83 Compare August 1, 2021 22:43
@hlovdal
Copy link
Contributor Author

hlovdal commented Aug 1, 2021

Branch updated to make use of iter_test, as well as adding/updating tests for each relevant commit.

@hlovdal hlovdal force-pushed the forget-all-argument branch from 5ffef83 to 24047cc Compare November 28, 2022 20:07
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.

2 participants