-
-
Notifications
You must be signed in to change notification settings - Fork 1k
added automatic release svg generation #2327
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
base: main
Are you sure you want to change the base?
Conversation
for more information, see https://pre-commit.ci
fixed colors
for more information, see https://pre-commit.ci
sarahboyce
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.
Thank you for the PR! This is really strong
I have added some suggestions to make it easier to generate future svgs and how to make the generated image a little closer to the existing image
I think once we're happy with the generated image, we might want to consider writing a few tests
Co-authored-by: Sarah Boyce <42296566+sarahboyce@users.noreply.github.com>
Co-authored-by: Sarah Boyce <42296566+sarahboyce@users.noreply.github.com>
for more information, see https://pre-commit.ci
for more information, see https://pre-commit.ci
|
Hi , thank you for the review . I have added suggested changes , please do let me know if anything needs to be added |
sarahboyce
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.
I have suggested changes which would end up generating the following image
After commiting suggestions you will need to update the current image committed by re-running the command.
A nice to have would be if we can end the graph one line earlier and add a fade out 🤔
We still have some minor discrepancies but I think it's "close enough"
sarahboyce
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.
Thank you for the updates @bhargav-j47 !
If we can have the "April", "August", "December" labels on being on the lines as in the original image, I think that would be amazing ✨
I think this is ready for a review from the website WG 👍
sarahboyce
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.
Just realized the LTS extended support is all about 8 months too short, can you update (also update the svg)?
Co-authored-by: Sarah Boyce <42296566+sarahboyce@users.noreply.github.com>
Co-authored-by: Sarah Boyce <42296566+sarahboyce@users.noreply.github.com>
tools/generate_release_roadmap.py
Outdated
| OUTPUT_FILE = os.path.join( | ||
| BASE_DIR, "..", "djangoproject", "static", "img", "release-roadmap.svg" | ||
| ) |
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.
Minor: Can we use pathlib.Path for all path manipulations in this file? This would let us use OUTPUT_file.write_text(...) later on.
tools/generate_release_roadmap.py
Outdated
| def add_months(date: dtime.date, months: int) -> dtime.date: | ||
| year = date.year + (date.month - 1 + months) // 12 | ||
| month = (date.month - 1 + months) % 12 + 1 | ||
| day = min(date.day, calendar.monthrange(year, month)[1]) | ||
| return dtime.date(year, month, day) |
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.
Minor: It seems that we already pull python-dateutil as a dependency somehow (looking at pip freeze | grep dateutil), so we could add it to our requirements file explicitly and use it here:
from dateutil.relativedelta import relativedelta
release_date + relativedelta(months=8)
tools/generate_release_roadmap.py
Outdated
| "--first-release", required=True, help="First release number, e.g., 4.2" | ||
| ) | ||
| parser.add_argument( | ||
| "--date", required=True, help="Release date in YYYY-MM format, e.g., 2023-04" |
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.
Suggestion: You could use argparse's type parameter to have this automatically be converted to a date object: https://docs.python.org/3/library/argparse.html#type
bmispelon
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.
I clicked the wrong button and left individual comments rather than a review 🤦🏻
I left a few suggestions, some stronger than others (the content type issue for the og_image should really be fixed, the others are optional).
I also noticed two visual differences with the previous png image, not sure if it's been discussed already but I thought I'd point them out:
- The legend texts ("Mainstream support" and "Extended support") are not centered anymore, they seem to be left-aligned
- The month names are now horizontal instead of vertical as they were before, was that intentional?
Finally, in terms of code layout I'm a bit wary about adding a new tools directory. Could we make this a management command inside the existing releases app?
Thanks again for your work! 🎸
for more information, see https://pre-commit.ci
for more information, see https://pre-commit.ci
|
i have commit suggested changes
i have also added svg generation as management command in releases app with usage "python -m manage generate_release_roadmap --first release --date |
|
Tested the management command - looks good to me! Please remove the tools directory 👍 |
bmispelon
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.
I've tested the management command and it works well. As Sarah pointed out, you can remove the tools directory now.
I've also left a small suggestion to simplify the use of OUTPUT_FILE now that it's a Path object.
Thanks again for working on this! 🎸
| with open(OUTPUT_FILE, "w", encoding="utf-8") as f: | ||
| f.write(output_svg) |
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.
Now that OUTPUT_FILE is a Path object, this can be replaced with:
| with open(OUTPUT_FILE, "w", encoding="utf-8") as f: | |
| f.write(output_svg) | |
| OUTPUT_FILE.write_text(output_svg) |
https://docs.python.org/3/library/pathlib.html#pathlib.Path.write_text
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.
removed tools directory and done suggested changes
|
i have done suggested changes please check if any more changes are needed |
this PR adds feature #2297, fixes #2297
PR introduces following changes:
Automates the generation of the release roadmap SVG using a Python script and json.
changes download page from release-roadmap.png to release-roadmap.svg