-
Notifications
You must be signed in to change notification settings - Fork 54
Shorten managed dependencies snapshot folder names #293
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
Francisco-Gamino
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.
One minor comment; otherwise, LGTM.
| /// ".../20190710-1234.567890.r.i" | ||
| /// This makes it possible to enumerate all the installed snapshots by using ".../*.r" mask, | ||
| /// and all the installing snapshots by using ".../*.i" mask. | ||
| /// ".../1907101234567ri" |
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.
Should this be 1907101234567i?
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.
@AnatoliB: Please rebase your code. The unsuccessful checks should pass now :).
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.
Should this be 1907101234567i?
It could, but the code will actually do 1907101234567ri, for the sake of simplifying the transformation from the "ready" name to the "installing" name. Currently, we just append i, which is a no-brainer. If we wanted to do 1907101234567i instead, we would have to do a replacement, make a decision on how to handle the situation when the original name does not end with "i". Though not very difficult, these decisions complicate the problem without a valuable reason.
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.
That makes sense. Thanks for the clarification @AnatoliB.
Shorten managed dependencies snapshot folder names: