Skip to content
This repository has been archived by the owner on Aug 4, 2023. It is now read-only.

Remove creation date from generated PDFs #4

Merged
merged 2 commits into from
Jun 27, 2021

Conversation

ericcornelissen
Copy link
Contributor

... in order to prevent the PDFs from "changing" when running npm run build for new/updated SVGs.

The implementation is a bit of a hack as it involves providing a value for the info.CreationDate option that results in an empty string in the PDF.

Note that with this Pull Request, the change hasn't taken effect yet. The PDFs need to be regenerated once in order for the creation date to be set to an empty string, after which they will stay that way and unchanged SVGs no longer result in new PDFs.

The creation date included in the generated PDFs causes `npm run build`
to update all PDFs with a new creation date, causing a lot of
unnecessary changes. This hacks the CreateDate value to be an empty
string, preventing regeneration of the PDFs from causing changes to all
PDFs.

This can be tested by running `npm run build` on the master branch
and observing that all PDFs are changed. Then running `npm run build` on
this branch, commiting the result, running `npm run build` again and
observering no changes.
@ericcornelissen ericcornelissen added the meta Issues or pull requests regarding the project or repository itself label Jun 24, 2021
@ericcornelissen ericcornelissen changed the title Remove creation date from genrated PDFs Remove creation date from generated PDFs Jun 24, 2021
Copy link
Member

@adamrusted adamrusted left a comment

Choose a reason for hiding this comment

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

This looks good to me, @ericcornelissen - and works perfectly when testing locally.
The only other proposal would be to set the creation date to the last time the SVG was modified on the main repo, but that's a headache for another PR!

@ericcornelissen
Copy link
Contributor Author

This looks good to me [...] and works perfectly when testing locally.

Thanks for verifying this 👍

The only other proposal would be to set the creation date to the last time the SVG was modified on the main repo, but that's a headache for another PR!

I totally agree that would be a nicer solution, and I thought about it myself as well, but I can't think of a way to get this information currently (the file stats of the npm installed SVGs does not contain the "real" last modified date) and I also don't think it makes sense to add a field for this to the simple-icons.json data file...

@adamrusted
Copy link
Member

I totally agree that would be a nicer solution, and I thought about it myself as well, but I can't think of a way to get this information currently (the file stats of the npm installed SVGs does not contain the "real" last modified date) and I also don't think it makes sense to add a field for this to the simple-icons.json data file...

That's fair. I imagine otherwise there's a LOT of logic to trying to find the commit that edited the file last, and using the timestamp of that commit. Not worth it in the grand scheme of things.

@ericcornelissen
Copy link
Contributor Author

It's definitely possible, but not worth the trouble I'd say. Not to mention that it would require fetching the repository on top of/instead of fetching the package from NPM.

I don't think that's worth the trouble of building and maintaining something for this here. Maybe we can do this if there's a pre-existing solution for analyzing the hit history like this 🤔

@adamrusted
Copy link
Member

Yeah that's fair, as I say it's something for another PR if we decide to proceed with it. Happy for this to get merged in, if it's ready?

@ericcornelissen
Copy link
Contributor Author

This PR is ready, but the website repo is not yet ready for this to be merged 🙃 hoping to get that done tomorrow.

@ericcornelissen
Copy link
Contributor Author

ericcornelissen commented Jun 27, 2021

Yeah that's fair, as I say it's something for another PR if we decide to proceed with it.

Do you want to open an issue for that, @adamrusted?

@ericcornelissen ericcornelissen marked this pull request as ready for review June 27, 2021 11:53
@ericcornelissen ericcornelissen merged commit 77931e0 into master Jun 27, 2021
@ericcornelissen ericcornelissen deleted the build/remove-date branch June 27, 2021 11:54
Sign up for free to subscribe to this conversation on GitHub. Already have an account? Sign in.
Labels
meta Issues or pull requests regarding the project or repository itself
Projects
None yet
Development

Successfully merging this pull request may close these issues.

None yet

2 participants