-
Notifications
You must be signed in to change notification settings - Fork 68
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
F todo UUID update test #1052
Open
davidkim1
wants to merge
4
commits into
regro:main
Choose a base branch
from
davidkim1:f_todo_uuid_update_test
base: main
Could not load branches
Branch not found: {{ refName }}
Loading
Could not load tags
Nothing to show
Loading
Are you sure you want to change the base?
Some commits from the old base branch may be removed from the timeline,
and old review comments may become outdated.
Open
F todo UUID update test #1052
Changes from 2 commits
Commits
Show all changes
4 commits
Select commit
Hold shift + click to select a range
94cf6b8
added test for f_todo with uuid to be marked as finished
davidkim1 c8d2542
added a test in helper_map in for f_todo that passed a uuid without a…
davidkim1 68a6347
added tests for f_todo with uuid passed as index
davidkim1 644a671
added logic to f_todo helpers to finish todo based on uuid
davidkim1 File filter
Filter by extension
Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
There are no files selected for viewing
This file contains bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
Oops, something went wrong.
Add this suggestion to a batch that can be applied as a single commit.
This suggestion is invalid because no changes were made to the code.
Suggestions cannot be applied while the pull request is closed.
Suggestions cannot be applied while viewing a subset of changes.
Only one suggestion per line can be applied in a batch.
Add this suggestion to a batch that can be applied as a single commit.
Applying suggestions on deleted lines is not supported.
You must change the existing code in this line in order to create a valid suggestion.
Outdated suggestions cannot be applied.
This suggestion has been applied or marked resolved.
Suggestions cannot be applied from pending reviews.
Suggestions cannot be applied on multi-line comments.
Suggestions cannot be applied while the pull request is queued to merge.
Suggestion cannot be applied right now. Please check back later.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
the index and uuid should be handled in the same way as each other as we discussed, so we probably either need to change how index is handled (remove optional argument --index and take is a required argument) or have the uuid taken in --index. I am not sure there is a way to test this unless we run the tests again from scratch. Maybe it is time to set that up.
Given that choice I would be inclined to pass the uuid as index. Maybe we change that variable name to --index_uuid or something like that? We will have to comment out the test.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I would be inclined to change variable name to index_uuid or adding a variable name uuid. Because it would be clearer to the user what to enter. If we change it to index_uuid it is not as clear because one can think it means entering the index and uuid. If we leave it as just index, I will change the definition of described in index to add that it is interchangeable with the first 6 characters of a todo's uuid.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
My idea is that the user can enter either an index or a uuid. Either would work in exactly the same way. If I enter
regolith helper f_todo --index 123
orregolith helper f_todo --index 8yt4q
will work the same way if todo with index 123 has uuid 8yt4q.Let's just keep the arg name as index for now. Late we can easily change it if we find a better name.