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

Revert sanitization of package name #7246

Merged
merged 1 commit into from
Jul 26, 2023

Conversation

deivid-rodriguez
Copy link
Contributor

I think this was a mistake from the beginning. The specs added after reverting the patch still pass without the change, and the commit message reads: "JS: Sanitize spaces in filenames", while there's no filename involved here whatsoever, just the package name.

This commit reverts c5b1fe6 and 2864404.

References: #7245 (comment).

@deivid-rodriguez
Copy link
Contributor Author

The removed tests still pass after the revert. Of course, because test npm and the removed code is for Yarn. Something got weirdly out of sync here. In any case, just to make sure I migrated the specs to use Yarn, and they still pass.

In any case, I think it's best to remove them to avoid any confusion, since there's nothing invalid (I believe) about a package name with spaces?

@deivid-rodriguez deivid-rodriguez marked this pull request as ready for review May 5, 2023 20:08
@deivid-rodriguez deivid-rodriguez requested a review from a team as a code owner May 5, 2023 20:08
@jeffwidman
Copy link
Member

🤔 Perhaps this was originally intended to sanitize the package name when it was used for the branch name?

Agree though that the place to handle that is at the branch name sanitizer, not here...

@deivid-rodriguez
Copy link
Contributor Author

That's a good point, but yeah if that was needed the place would be wrong. And why only for Yarn? 🤷‍♂️

@deivid-rodriguez deivid-rodriguez force-pushed the deivid-rodriguez/revert-package-name-sanitization branch from b7c70df to 0fbcbb1 Compare May 12, 2023 10:56
@jeffwidman jeffwidman force-pushed the deivid-rodriguez/revert-package-name-sanitization branch from 0fbcbb1 to e024f35 Compare July 25, 2023 22:27
I think this was a mistake from the beginning. The specs added after
reverting the patch still pass without the change, and the commit
message reads: "JS: Sanitize spaces in filenames", while there's no
filename involved here whatsoever, just the package name.

This commit reverts c5b1fe6 and
2864404.
@jeffwidman jeffwidman force-pushed the deivid-rodriguez/revert-package-name-sanitization branch from e024f35 to 1ed2478 Compare July 26, 2023 16:54
@jeffwidman
Copy link
Member

Checked with Deivid on slack and he thought this was good to go, so going to merge/deploy it.

@jeffwidman jeffwidman enabled auto-merge (squash) July 26, 2023 16:55
@jeffwidman jeffwidman merged commit 40c0d20 into main Jul 26, 2023
90 checks passed
@jeffwidman jeffwidman deleted the deivid-rodriguez/revert-package-name-sanitization branch July 26, 2023 17:05
brettfo pushed a commit to brettfo/dependabot-core that referenced this pull request Oct 11, 2023
I think this was a mistake from the beginning. The specs added after
reverting the patch still pass without the change, and the commit
message reads: "JS: Sanitize spaces in filenames", while there's no
filename involved here whatsoever, just the package name.

This commit reverts c5b1fe6 and
2864404.
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Projects
None yet
Development

Successfully merging this pull request may close these issues.

None yet

3 participants