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

Maintenance: make time conversion script platform-independent #373

Merged
merged 4 commits into from
Dec 12, 2023

Conversation

SidestreamColdMelon
Copy link
Contributor

This PR replaces time.sh script with the equivalent time.py script. The functionality and the interface of the new script stays exactly the same, but now it can be run across platforms. In comparison, the previous script didn't work on macOS and after a trial to fix it (see d623861) I decided to rewrite it in python to avoid inconsistencies.

Copy link
Contributor

@amusingaxl amusingaxl left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

The scripts are not equivalent.

With the bash version, if I run:

make time date="2023-12-01T00:00:00Z"

I get:

Fri Dec  1 12:00:00 AM UTC 2023
1701388800

However, the python script does not work with the same arguments:

Traceback (most recent call last):
  File "/home/anon/work/makerdao/spells-mainnet/./scripts/time.py", line 20, in <module>
    utc_date = datetime.strptime(date, DATE_FORMAT).replace(tzinfo=timezone.utc)
               ^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^
  File "/usr/lib/python3.11/_strptime.py", line 568, in _strptime_datetime
    tt, fraction, gmtoff_fraction = _strptime(data_string, format)
                                    ^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^
  File "/usr/lib/python3.11/_strptime.py", line 349, in _strptime
    raise ValueError("time data %r does not match format %r" %
ValueError: time data '2023-12-01T00:00:00Z' does not match format '%Y-%m-%d %H:%M:%S'
make: *** [Makefile:21: time] Error 1

We should probably be less strict about date parsing. At least we should allow ISO8601 strings like the one above.

@amusingaxl
Copy link
Contributor

Looks like there's a good way to parse ISO8601 dates in Python 3:
https://stackoverflow.com/a/49784038/1798341

@SidestreamColdMelon
Copy link
Contributor Author

That was actually my undocumented intention to make script less smart and more strict with parsing. But supporting a single standard like ISO8601 is also a good choice. I've updated the script!

@amusingaxl
Copy link
Contributor

LGTM.
Should we also port this to Goerli?
It will be deprecated soon, and we don't have many uses for it there, but it's fairly a small change.
https://github.com/makerdao/spells-goerli/blob/master/scripts/time.sh

@SidestreamColdMelon SidestreamColdMelon merged commit 7400e91 into master Dec 12, 2023
@SidestreamColdMelon SidestreamColdMelon deleted the time-script-maintenance branch December 12, 2023 08:30
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.

3 participants