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

Add missing entries. #10

Closed
wants to merge 1 commit into from
Closed

Add missing entries. #10

wants to merge 1 commit into from

Conversation

Potherca
Copy link
Member

As discovered during #7, some .pngs that have a *.puml counterparts are not listed in sprites-list.md.

This MR fixes that, so we can release v1.1.0, and take our time in fixing the root problem.

@Potherca Potherca added this to the v1.1.0 milestone May 19, 2023
@Potherca Potherca requested a review from rabelenda May 19, 2023 14:32
@Potherca Potherca self-assigned this May 19, 2023
@Potherca Potherca mentioned this pull request May 19, 2023
7 tasks
@rabelenda
Copy link
Collaborator

Hello @Potherca,

I see the changes in sprites-list.md, but not changes in the groovy script. Is this ok?

Have you just re-run the groovy script to regenerate the list? Or have you fixed something in groovy script which is missing in this MR?

Regards

@Potherca
Copy link
Member Author

I cheated and created the list in bash, from the /pngs/ directory. I thought, if we merge this first we can release v1.1 and take our time in finding and fixing the issue in groovy.

@rabelenda
Copy link
Collaborator

Hello, I think you did a good job fixing the listing, but I would avoid leaving something inconsistent (not documented or automated approach), otherwise, we may not remember in the future how we got the listing "working", if we ever want to revisit our steps or want to release a new version.

I have created this branch where I have updated to use groovy 4 and latest dependencies, and most of the listing is complete. There are some missing entries because the logos are no longer in gilbarbara/logos repo or have changed names. If you want, after review, you, or me, can just merge it into master.

I think we should add to the script to remove them from this repo when that happens (is just adding logic to clean pngs and pumls folders at script initialization). That would imply that if some user doesn't use a tag, and use master, then diagrams using such logos may stop "working", but in general I think users should just use tagged versions.

What do you think?

@Potherca
Copy link
Member Author

I would avoid leaving something inconsistent

Fair enough. 👍

There are some missing entries because the logos are no longer in gilbarbara/logos repo or have changed names.
[...]
I think we should add to the script to remove them from this repo when that happens (is just adding logic to clean pngs and pumls folders at script initialization).

We could leave the removed logos in (to avoid backward incompatibility). That would require other logic. It would make it possible to mark logos that have been removed/renamed upstream in the markdown file. Also, maybe there is a way of marking things as deprecated, instead of directly removing them. Not sure.

That would imply that if some user doesn't use a tag, and use master, then diagrams using such logos may stop "working", but in general I think users should just use tagged versions.

The issue here is when users don't include this project using a URL (with a version) but through PlantUML directy (with the <sprites/foo> syntax).

It such cases, removing things will cause unexpected breakage for users.

It is possible adding "versioning" by using folders which include a version number (like awslib, awslib10, and awslib14) but that is something I would rather avoid if possible.

@rabelenda
Copy link
Collaborator

rabelenda commented May 27, 2023

I have added to the branch logic to mark deprecated sprites. Let me know if it looks good to you, mainly checking generated listing. If so, you (or me) can merge the changes and tag the release.

@rabelenda
Copy link
Collaborator

I just took the liberty to merge the changes into master branch and create the release 1.1. Feel free to modify anything, or let me know of any comments you may have. Thank you @Potherca for helping with this and taking the initiative.

@Potherca
Copy link
Member Author

Closing this in favor of fixing things in the script as suggested by @rabelenda.

Thank you @Potherca for helping with this and taking the initiative

You are very welcome! Glad to see combining our efforts into one plantuml-stdlib org seems to be working! 👍

@Potherca Potherca closed this May 29, 2023
@Potherca Potherca deleted the fix/sprites-list.md branch May 29, 2023 14:29
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.

2 participants