search

mdn / yari

The platform code behind MDN Web Docs
Mozilla Public License 2.0
1.19k stars 508 forks source link

Filecheck and savecompress should be run as part of flaw checking #2969

Closed hamishwillee closed 1 year ago

hamishwillee commented 3 years ago

Given that an uncompressed file will stop fail PR acceptance tests in CI it would be much better if uncompressed files were reported as flaws and I was given the opportunity to fix them before submitting the PR.

Note, I've reported this before, but only as part of other issues.

peterbe commented 3 years ago

I suspect this is a duplicate of another issue filed as an "enhancement" or "post-launch" issue. You're suggesting that the flaw checker should also do what the file-checker does so that you're alerted before you make the PR.

hamishwillee commented 3 years ago

Yes.

Fixing these as a flaw would save quite a bit of time.

Typical cycle right now is ... after finishing a PR, uploading, and indulging in some FIGJAM I get an email telling me one of the images I uploaded needs to be compressed. I fix it and reupload. Then I get a second notification for another image I forgot to compress. Repeat.

hamishwillee commented 2 years ago

@schalkneethling Bunch of yari cases appear to have been closed too. This is still IMO valid.

schalkneethling commented 2 years ago

@schalkneethling Bunch of yari cases appear to have been closed too. This is still IMO valid.

Yup, I have reopened a bunch a bit earlier. Please do let me know if there are ones I missed. Thanks.

caugner commented 1 year ago

Our current plan is to actually move checks from flaws (build-time) to filecheck (on-demand) to reduce our build time, so we don't plan on doing the inverse (moving filecheck into flaws). In the long-term, filecheck would be the same as flaws and independent from our build, and we could possibly even run it during pre-commit like we run other linters.

hamishwillee commented 1 year ago

The problem now is that you submit your PR you don't find out about the problem until tests fail. This has cost me a heap of time. I think you are saying that I will be able to submit any old file size and there won't be any tests any more pre-submission? If so, thanks - that would be much less irritating.

and we could possibly even run it during pre-commit like we run other linters.

And of course this would be even better ^^^

caugner commented 1 year ago

And of course this would be even better ^^^

@hamishwillee FYI Since September 2022 (https://github.com/mdn/content/pull/21009), yarn filecheck is actually run on images to be committed as part of the pre-commit hook:

https://github.com/mdn/content/blob/0882acacfbe8bded94c00f743b1756cd914856e6/.lintstagedrc.json#L4

So if you commit an image on the command line and it is too big or can be significantly compressed, it should prevent you from committing. 🎉

(And once https://github.com/mdn/yari/pull/7716 lands, the UX will be much better.)

hamishwillee commented 1 year ago

Thanks, I guess I haven't added any images since September!

So if you commit an image on the command line and it is too big or can be significantly compressed, it should prevent you from

Given that at this point there is nothing a user can do to commit except compress the file, can the linter also run --save-compress?