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
Override VCSDownloadStrategy#source_modified_time #171
Override VCSDownloadStrategy#source_modified_time #171
Conversation
LGTM 👍 @xu-cheng? |
Not being very familiar with the listed tools, is there some way to make them output the time in a more standardized way (e.g. ISO 8601, RFC 2822) like we do with the Git download strategy? It would be a pity if parsing failed because of version-dependent changes in the human-readable output format that is currently being parsed or if the time format adapts to the user's locale. Have you checked what happens when you set, e.g., |
25d1c71
to
68ac049
Compare
@UniqMartin I modified the options a little, I guess this is all I can do here. It'll output dates in UTC format, but I cannot manage to output it in a more standartised way.
Doesn't change anything. |
@@ -830,6 +838,10 @@ def stage | |||
safe_system(*args) | |||
end | |||
|
|||
def source_modified_time | |||
Time.parse Utils.popen_read("fossil", "info", "tip", "-R", cached_location.to_s)[/^parent: *\h* (.*)$/, 1] |
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 think it would be good to make the pattern
^parent: +\h+ (.+)$
instead of^parent: *\h* (.*)$
. Judging from the example output it seems reasonable to expect that there is always whitespace after the colon, always a hexadecimal ID, and always a non-empty date/time. - Looking at the example output you provided makes me think we should be parsing the
uuid:
line instead of theparent:
line. I tried to confirm that by searching Fossil's online documentation, but failed to find a definitive answer. Can you check that and/or point me at the relevant documentation?
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.
@UniqMartin agree on regex.
Thanks a lot about parent
and uuid
stuff, I can't explain why I wrote parent
instead of uuid
. It's something wrong with me.
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.
It's something wrong with me.
There's nothing wrong with you! That's just human and happens to all of us all the time. 😸
Thanks for checking and adjusting your code where necessary!
Great! That's reassuring. |
68ac049
to
4862ae8
Compare
👍 Looks good to me; also all of my comments have been addressed. |
Thanks everyone. |
brew tests
with your changes locally?Detaills
Implement
MercurialDownloadStrategy#source_modified_time
BazaarDownloadStrategy#source_modified_time
FossilDownloadStrategy#source_modified_time
from #81
Bazaar
bzr log -l 1
has the following format:Mercurial
hg tip
has the following format:hg log -l 1
also possible, buthg tip
seems to be better.Fossil
fossil info tip -R <repo>
has the following format: