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

Remove unnecessary validation #2043

Open
wants to merge 4 commits into
base: main
Choose a base branch
from

Conversation

Pickysaurus
Copy link
Contributor

There is a weird validation for images used in the changelog that tried to solve a problem that doesn't exist and simply creates friction/inconvenience when writing changelogs. Removed this check. We can re-add it if this becomes a problem that needs solving.

…s attempts to solve a problem that simply does not exist and becomes a blocker for timely updates of changelog content.
@Al12rs
Copy link
Contributor

Al12rs commented Sep 16, 2024

Personally I agree. I think the goal of the extra steps is to ensure that no large images are added to the Git history, which would increase the repo size irreversibly (unless history is altered).
But this makes working on it too cumbersome.

@Pickysaurus
Copy link
Contributor Author

Personally I agree. I think the goal of the extra steps is to ensure that no large images are added to the Git history, which would increase the repo size irreversibly (unless history is altered). But this makes working on it too cumbersome.

We can still require they be converted to WebP but the naming rules just serve to be a pain IMHO

@halgari
Copy link
Collaborator

halgari commented Sep 16, 2024

It's probably best to address the original rationale for the images being hashed.

As stated in the doc:

  • Prevents duplicate files.
  • Prevents duplicate file names.
  • Prevents uninformative file names.

These seem like useful conerns, based on my previous experience in this area. Most of the time english names for images devolve into useless or generic names side_bar-example-img42.png. This cuts out a lot of that guess work by letting us use images as-is. Granted the scripts here are not written for use on Windows and we should probably fix that if we need to be running those locally.

If we just need to hash the file, that's easily done online: https://emn178.github.io/online-tools/blake2b/file/

@Pickysaurus
Copy link
Contributor Author

Pickysaurus commented Sep 17, 2024

It's probably best to address the original rationale for the images being hashed.

As stated in the doc:

  • Prevents duplicate files.
  • Prevents duplicate file names.
  • Prevents uninformative file names.

These seem like useful conerns, based on my previous experience in this area. Most of the time english names for images devolve into useless or generic names side_bar-example-img42.png. This cuts out a lot of that guess work by letting us use images as-is. Granted the scripts here are not written for use on Windows and we should probably fix that if we need to be running those locally.

If we just need to hash the file, that's easily done online: https://emn178.github.io/online-tools/blake2b/file/

Honestly, there aren't problems we've actually had at any point so I don't understand why there's an overkill process (that is the opposite of intuitive).

Prevents duplicate files.

This doesn't prevent that (unless the repo is made case sensitive too?). Also, if we're ensuring everything is WEBP I don't see this being a problem when it comes to storage.

Prevents duplicate file names.

Windows already does this. We should not be hamstrung because of Linux oddities. I believe you can set the paths on the repo to be case-sensitive even on Linux if it's a problem.

Prevents uninformative file names.

How are random hashes of nonsense informative? Why does it matter? It's an image. You can open the file if you're unclear on what it is. I'd probably suggest we have a simpler convention like prefixing the image file with the version number it was added for.

To be frank, if this process remains I may just host my changelog images elsewhere as it's far less hassle.

@halgari
Copy link
Collaborator

halgari commented Sep 19, 2024

It's probably best to address the original rationale for the images being hashed.
As stated in the doc:

  • Prevents duplicate files.
  • Prevents duplicate file names.
  • Prevents uninformative file names.

These seem like useful conerns, based on my previous experience in this area. Most of the time english names for images devolve into useless or generic names side_bar-example-img42.png. This cuts out a lot of that guess work by letting us use images as-is. Granted the scripts here are not written for use on Windows and we should probably fix that if we need to be running those locally.
If we just need to hash the file, that's easily done online: https://emn178.github.io/online-tools/blake2b/file/

Honestly, there aren't problems we've actually had at any point so I don't understand why there's an overkill process (that is the opposite of intuitive).

These are issues I've had in almost every documentation project I've worked on. Files start with names like sensible_image.png and then eventually devolve into sensible_img.png, sensible-image.png, 00-sensible_image.png. It quickly becomes a situation where the names are completely pointless anyways and doc writers have to do visual searching to find an image they want. Name hashes aren't worse in this respect, and they are better in others.

Prevents duplicate files.

This doesn't prevent that (unless the repo is made case sensitive too?). Also, if we're ensuring everything is WEBP I don't see this being a problem when it comes to storage.

Duplicate images (like a logo used in several places) will be hashed into the same name, and then be de-dupped.

Prevents duplicate file names.

Windows already does this. We should not be hamstrung because of Linux oddities. I believe you can set the paths on the repo to be case-sensitive even on Linux if it's a problem.

Every operating system does this, but what they don't de-dupe is logical name duplication like foo_bar.png and foo-bar.png, or img vs image.

Prevents uninformative file names.

How are random hashes of nonsense informative? Why does it matter? It's an image. You can open the file if you're unclear on what it is. I'd probably suggest we have a simpler convention like prefixing the image file with the version number it was added for.

It isn't any better in that respact, but they are at least uniform.

To be frank, if this process remains I may just host my changelog images elsewhere as it's far less hassle.

Well we should improve the process, and make it easier to contribute. But the images should remain in the main repo.

@Pickysaurus
Copy link
Contributor Author

This continues to cause unnecessary problems and frustration.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
Status: Review
Development

Successfully merging this pull request may close these issues.

3 participants