Skip to content

Fix svgo workflow for ptable scatter plots #187

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.

Sign up for GitHub

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

Jump to bottom
Merged
merged 11 commits into from
Aug 18, 2024
Merged

Fix svgo workflow for ptable scatter plots #187

merged 11 commits into from
Aug 18, 2024

Conversation

DanielYang59
Copy link
Collaborator

@DanielYang59 DanielYang59 commented Jul 29, 2024 •
edited
Loading

Summary

The original commit when the issue was first noticed: #165

@DanielYang59 DanielYang59 self-assigned this Jul 29, 2024
@DanielYang59 DanielYang59 added help wanted Extra attention is needed fix Bug fix PRs ci Continuous integration labels Jul 29, 2024
@janosh
Copy link
Owner

janosh commented Aug 6, 2024

can this PR be closed?

@DanielYang59
Copy link
Collaborator Author

I cannot recreate this issue for some reason, tried to enable svgo workflow for scatters in this PR, but didn't see the scatter plots being corrupted.

@janosh
Copy link
Owner

janosh commented Aug 7, 2024

maybe because it's a newer version of svgo and they fixed the issue in the meantime? if that's the case, we can remove the --exlude flag

@DanielYang59 DanielYang59 marked this pull request as ready for review August 8, 2024 09:41
@DanielYang59
Copy link
Collaborator Author

maybe because it's a newer version of svgo and they fixed the issue in the meantime?

No idea. And the workflow doesn't show svgo version so I cannot recreate that (by using the same version as that broken plots). Therefore added a debug command to show svgo version when installed. Hopefully it would help us catch that svgo version should this happen again, or if don't it's better and we could remove that tag.

@DanielYang59
Copy link
Collaborator Author

@janosh Perhaps we could merge this now until (hopefully never) this issue pop up again?

DanielYang59
DanielYang59 commented Aug 18, 2024
View reviewed changes
.github/workflows/svgo.yml Outdated
if git diff --quiet assets; then
echo "No changes to commit"
exit 0
fi
git config user.name "Janosh Riebesell"
git config user.email [email protected]
git add assets
git commit -m "compress new SVG assets"
git commit -m "[workflow] Compress new SVG assets"
Copy link
Collaborator Author

Choose a reason for hiding this comment

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

Added this prefix to distinguish between a commit added by automatic workflow (I thought the original commit was done by you yourself)

Copy link
Owner

Choose a reason for hiding this comment

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

good idea

@DanielYang59 DanielYang59 removed the help wanted Extra attention is needed label Aug 18, 2024
@janosh janosh merged commit 8700d3b into main Aug 18, 2024
7 checks passed
@janosh janosh deleted the fix-svgo-compression-workflow branch August 18, 2024 05:46
@DanielYang59
Copy link
Collaborator Author

Thanks!

janosh added a commit that referenced this pull request Mar 28, 2025
@DanielYang59 @janosh
Fix svgo workflow for ptable scatter plots (#187)
56fa68e 
* trigger svgo workflow

* compress new SVG assets

* Revert "compress new SVG assets"

This reverts commit 9b37f26.

* Revert "trigger svgo workflow"

This reverts commit 2b2bdc1.

* remove scatter plot exclusion

* add debug tag: show svgo version

* add remove tag

* combine svgo install and run step

* add some spaces

* git commit -m "[workflow->ci] Compress new SVG assets"

---------

Co-authored-by: Janosh Riebesell <[email protected]>
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Reviewers

@janosh janosh janosh left review comments

Assignees

@DanielYang59 DanielYang59

Labels
ci Continuous integration fix Bug fix PRs
Projects
None yet
Milestone
No milestone
Development

Successfully merging this pull request may close these issues.

svgo might break ptable scatter plots
2 participants
@DanielYang59 @janosh