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.

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

fix: Replace broken goo.gl link with actual docs link #14081

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

fix: Replace broken goo.gl link with actual docs link #14081

wants to merge 5 commits into from

Conversation

legobeat
Copy link

Summary

  • Unobfuscate destination URL
  • Remove need for user to access Google services to discover relevant docs
  • Future-proof - no risk of sunsetting or unavailability of intermediary service causing breakage

Test plan

@facebook-github-bot
Copy link
Contributor

Hi @legobeat!

Thank you for your pull request and welcome to our community.

Action Required

In order to merge any pull request (code, docs, etc.), we require contributors to sign our Contributor License Agreement, and we don't seem to have one on file for you.

Process

In order for us to review and merge your suggested changes, please sign at https://code.facebook.com/cla. If you are contributing on behalf of someone else (eg your employer), the individual CLA may not be sufficient and your employer may need to sign the corporate CLA.

Once the CLA is signed, our tooling will perform checks and validations. Afterwards, the pull request will be tagged with CLA signed. The tagging process may take up to 1 hour after signing. Please give it that time before contacting us about it.

If you have received this in error or have any questions, please contact us at cla@meta.com. Thanks!

@facebook-github-bot
Copy link
Contributor

Thank you for signing our Contributor License Agreement. We can now accept your code for this (and any) Meta Open Source project. Thanks!

@legobeat legobeat marked this pull request as ready for review April 18, 2023 04:48
@legobeat legobeat mentioned this pull request Apr 18, 2023
Merged
SimenB
SimenB requested changes Apr 23, 2023
View reviewed changes
Copy link
Member

@SimenB SimenB left a comment

Choose a reason for hiding this comment

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

this is a breaking change, so cannot be done until the next major.

the URL also seems broken - it's supposed to point to https://jestjs.io/docs/snapshot-testing

@SimenB
Copy link
Member

SimenB commented Apr 23, 2023

/easycla

@linux-foundation-easycla
Copy link

linux-foundation-easycla bot commented Apr 23, 2023 •
edited
Loading

CLA Signed

The committers listed above are authorized under a signed CLA.

@SimenB SimenB removed the cla signed label Apr 23, 2023
@SimenB SimenB added this to the Jest 30 milestone Apr 23, 2023
@legobeat
Copy link
Author

legobeat commented Apr 23, 2023 •
edited
Loading

@SimenB How would this be breaking? Fixed the broken link.

@legobeat legobeat changed the title chore: Replace shortened goo.gl link with actual docs link chore: Replace broken goo.gl link with actual docs link Apr 23, 2023
@legobeat legobeat changed the title chore: Replace broken goo.gl link with actual docs link fix: Replace broken goo.gl link with actual docs link Apr 23, 2023
@netlify
Copy link

netlify bot commented Apr 23, 2023 •
edited
Loading

✅ Deploy Preview for jestjs ready!

Built without sensitive environment variables

Name Link
🔨 Latest commit 864f06e
🔍 Latest deploy log https://app.netlify.com/sites/jestjs/deploys/66263177f2610f0008b9b015
😎 Deploy Preview https://deploy-preview-14081--jestjs.netlify.app
📱 Preview on mobile
Toggle QR Code...

QR Code

Use your smartphone camera to open QR code link.

To edit notification comments on pull requests, go to your Netlify site configuration.

@legobeat legobeat requested a review from SimenB April 23, 2023 12:22
@mrazauskas
Copy link
Contributor

How would this be breaking? Fixed the broken link.

Only in this PR almost 150 snapshots are not passing. Now imagine Renovate bot upgrading Jest: all snapshot users will get red CIs (because snapshots are not updated in CI). This simply should not happen between minor releases.

@mrazauskas
Copy link
Contributor

By the way, you should include change log entry too.

@legobeat
Copy link
Author

If CI indeed breaks, it would be very helpful if the workflow could be approved.

@legobeat
Copy link
Author

By the way, you should include change log entry too.

Where should a log be added, apart from bddff35?

@mrazauskas
Copy link
Contributor

mrazauskas commented Apr 23, 2023 •
edited
Loading

If CI indeed breaks, it would be very helpful if the workflow could be approved.

Do you speak about the CI in this repo? This will pass. I had in mind the CIs which run Jest for users in repos which are being updated by bots like Renovate.

@legobeat
Copy link
Author

legobeat commented Apr 23, 2023 •
edited
Loading

Hm, wouldn't the same hold f.e. for these, which were treated as non-breaking?

f2f578f (#12303)
d106643 (#14073)
e3d5491 (#10982)

@mrazauskas
Copy link
Contributor

mrazauskas commented Apr 23, 2023 •
edited
Loading

These two are changing something in this repo only (because lock file or snapshots are not shipped to NPM). The change you are making will be shipped to the users.

In other other words, the changes you are referring have zero impact on code of jest which people install from NPM, but your change will impact everyone.

@legobeat legobeat requested a review from mrazauskas April 23, 2023 13:05
@legobeat legobeat force-pushed the clear-link branch 2 times, most recently from c58cffa to aced412 Compare April 23, 2023 13:23
@mrazauskas
Copy link
Contributor

Looks good for my eye. Thanks!

@legobeat
Copy link
Author

Fixed changelog conflict.

SimenB
SimenB approved these changes May 12, 2023
View reviewed changes
Copy link
Member

@SimenB SimenB left a comment

Choose a reason for hiding this comment

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

thanks!

You don't have to keep this conflict free btw - I'll deal with it when I start landing breaking changes for v30 🙂

@legobeat legobeat requested a review from SimenB November 11, 2023 03:18
@polamjag
Copy link

Any updates? Google announced that they'll abondon all goo.gl links after August 25th, 2025: Google URL Shortener links will no longer be available - Google Developers Blog

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Reviewers

@mrazauskas mrazauskas mrazauskas left review comments

@SimenB SimenB Awaiting requested review from SimenB

Assignees
No one assigned
Labels
None yet
Projects
None yet
Milestone
Jest 30
Development

Successfully merging this pull request may close these issues.
5 participants
@legobeat @facebook-github-bot @SimenB @mrazauskas @polamjag