Skip to content

Conversation

markovejnovic
Copy link
Contributor

Fixes #213.

Tests pass locally:
image

@markovejnovic markovejnovic requested a review from a team as a code owner October 7, 2025 22:03
@markovejnovic markovejnovic force-pushed the user/markovejnovic/correctprotocol-fix branch 2 times, most recently from 6dd1003 to d68ba38 Compare October 7, 2025 22:11
@markovejnovic
Copy link
Contributor Author

I've also added a new test which fails on mainline but succeeds with this patch. I don't really know if this is a desirable fix, but my proposed implementation feels more correct than what we have in mainline.

@markovejnovic markovejnovic force-pushed the user/markovejnovic/correctprotocol-fix branch from d68ba38 to 7b273bb Compare October 7, 2025 22:38
@wraithgar wraithgar self-assigned this Oct 8, 2025
@wraithgar
Copy link
Member

Since the whole purpose of the function is to add the // to shorthand urls, this change makes sense: giving precedence to things that already have the // first.

However I fail to see what it has to do with #213.

@markovejnovic
Copy link
Contributor Author

Hi @wraithgar, thanks for the prompt response.

Sorry, I accidentally opened the ticket on the npm-package-arg repo: npm/npm-package-arg#213

Here's the relevant ticket: #315

@markovejnovic
Copy link
Contributor Author

@wraithgar, I've also added some comments to help clear up what each of the branches is responsible for. I can revert that change if you prefer, feel free to let me know

lib/parse-url.js Outdated
Comment on lines 37 to 38
// The URL has the form of <foo>@<bar> which is likely a shortcut URL
// such as @npm/cli
Copy link
Contributor Author

Choose a reason for hiding this comment

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

I'm not 100% sure my understanding behind the intent here is correct

Copy link
Member

Choose a reason for hiding this comment

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

This is for shorthand like: [email protected]:npm/hosted-git-info.git

@wraithgar
Copy link
Member

I've also added some comments to help clear up what each of the branches is responsible for. I can revert that change if you prefer, feel free to let me know

Comments are fine! Please don't length wrap them though, it's less accessible. Rule of thumb is markdown, comments, and strings for output shouldn't wrap. It's much easier to handle with a screen reader that way.

@markovejnovic markovejnovic force-pushed the user/markovejnovic/correctprotocol-fix branch from 6fdde50 to 65e21d0 Compare October 8, 2025 16:11
@markovejnovic
Copy link
Contributor Author

I've also added some comments to help clear up what each of the branches is responsible for. I can revert that change if you prefer, feel free to let me know

Comments are fine! Please don't length wrap them though, it's less accessible. Rule of thumb is markdown, comments, and strings for output shouldn't wrap. It's much easier to handle with a screen reader that way.

TIL, thanks! Also comments were too verbose so I slimmed them down.

@wraithgar wraithgar merged commit c91490e into npm:main Oct 8, 2025
17 checks passed
@github-actions github-actions bot mentioned this pull request Oct 8, 2025
wraithgar pushed a commit that referenced this pull request Oct 8, 2025
🤖 I have created a release *beep* *boop*
---


##
[9.0.2](v9.0.1...v9.0.2)
(2025-10-08)
### Bug Fixes
*
[`c91490e`](c91490e)
[#314](#314) correctProtocol
misparsing protocol (#314) (@markovejnovic)

---
This PR was generated with [Release
Please](https://github.com/googleapis/release-please). See
[documentation](https://github.com/googleapis/release-please#release-please).

Co-authored-by: github-actions[bot] <41898282+github-actions[bot]@users.noreply.github.com>
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Labels

None yet

Projects

None yet

Development

Successfully merging this pull request may close these issues.

2 participants