-
Notifications
You must be signed in to change notification settings - Fork 13
Feature/integ 1064 multi checkpoint #106
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
Conversation
| from code42cli.util import get_user_project_path | ||
|
|
||
|
|
||
| class Cursor(object): |
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.
This file contains the bulk of the change, most of the rest is formatting and renaming things / making the cli param pass a string instead of being a bool, etc
|
The amount of formatting changes in this PR tells me that stuff is getting committed without using |
| @@ -0,0 +1,175 @@ | |||
| from os import path | |||
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.
These are the new tests, the rest changed what the parameter name was and to support string vs bool, etc.
timabrmsn
left a comment
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.
LGTM
|
|
||
| @property | ||
| def value(self): | ||
| with (open(self._location)) as checkpoint: |
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.
Is there a reason for the parens around the open func? I've never seen that done before.
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.
nope, the reason you haven't seen it before is probably because it's totally unnecessary
Implements the ability to use multiple checkpoints for a single profile.
This is a breaking change --
-iwill no longer work, and has been replaced with-c(--use-checkpoint), which accepts a string name for the checkpoint.clear-checkpointwill also no longer work since it now requires a checkpoint name to be passed to it.Notes:
In the final refactoring, we want
checkpoint clear <name>(rather thanclear checkpoint <name>andcheckpoint listcommands. I did not implement those here since that work would be completely undone by the click refactor anyway, but their backends are in place (CursorStore.get_all_cursorshas basically everything needed for that)One issue that I'm aware of :because we keep this info on the file system now, it matters what you name your profile and/or checkpoint. For example if you name one
../../etc/password, we would end up trying to write to that file, which would be pretty bad, but I'm making a separate work item for that to be handled on its own.