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

fix: remove sourcehut bugstemplate #213

Merged
merged 1 commit into from
Sep 13, 2023
Merged

Conversation

vladh
Copy link
Contributor

@vladh vladh commented Sep 12, 2023

Currently, using https://git.sr.ht/~user/repo in npm init results in the package.json file having a bugs URL of
https://todo.sr.ht/~user/repo. However, this is not how SourceHut works. An issue tracker need not exist at that URL, and often projects have issue trackers that are named differently from the repository. Therefore, no information can be inferred about the issue tracker from the git URL.

@vladh vladh requested a review from a team as a code owner September 12, 2023 10:49
@vladh vladh changed the title sourcehut: remove bugstemplate fix: remove sourcehut bugstemplate Sep 12, 2023
@wraithgar
Copy link
Member

I think this is a compromise because some value needs to be here, npm bugs assumes that this module returns something here as long as it returns anything. The paradigm has been to resort to the repo url if one is not available.

@vladh
Copy link
Contributor Author

vladh commented Sep 12, 2023

I think this is a compromise because some value needs to be here, npm bugs assumes that this module returns something here as long as it returns anything. The paradigm has been to resort to the repo url if one is not available.

@wraithgar Then we should fix npm bugs (I'm happy to do so). Returning a nonsense value isn't a good option.

Do you have any pointers for what about npm should be tested here? I tried npm init with a SourceHut URL after applying my patch and I got the expected behaviour, i.e. no bugs URL being present in my package.json.

Currently, using https://git.sr.ht/~user/repo in `npm init` results in
the package.json file having a `bugs` URL of
https://todo.sr.ht/~user/repo. However, this is not how SourceHut works.
An issue tracker need not exist at that URL, and often projects have
issue trackers that are named differently from the repository.
Therefore, no information can be inferred about the issue tracker from
the git URL.
@vladh vladh force-pushed the fix-sourcehut-bugs-url branch from 677c6ce to 2e18d3b Compare September 12, 2023 16:08
@vladh
Copy link
Contributor Author

vladh commented Sep 12, 2023

I've found the issue I think you're referring to. After applying this patch and running npm bugs on a repo with a SourceHut git URL in package.json, I get the following error:

$ npm bugs
npm ERR! Invalid URL: undefined

npm ERR! A complete log of this run can be found in: /home/vladh/.local/state/npm/_logs/2023-09-12T16_10_33_189Z-debug-0.log

I'll send a npm bugs patch to fix this, then leave a comment here with a reference.

@wraithgar
Copy link
Member

Don't worry about the CI failures in node 16 et al, it's a disconnect between the npm version we're using in tests and the node version we test THIS module on. If tests pass in node 18.x we're good.

vladh added a commit to vladh/cli that referenced this pull request Sep 12, 2023
Currently, if the bugs URL returned from `package.json`, which is
processed through
[hosted-git-info](https://github.com/npm/hosted-git-info), is falsy, an
error is thrown. However, hosted-git-info may well return a falsy bugs
URL. This can happen when there is no bugs URL in the repo, but
hosted-git-info tries to infer one, but one cannot be inferred, for
example if the repository URL is a SourceHut URL. This is described
here: npm/hosted-git-info#213

For this reason, we should tolerate falsy URLs and fall back to whatever
is defined in `bugs.js`, in this case being sent to
https://www.npmjs.com/package/<packageName>.
vladh added a commit to vladh/cli that referenced this pull request Sep 12, 2023
Currently, if the bugs URL returned from `package.json`, which is
processed through
[hosted-git-info](https://github.com/npm/hosted-git-info), is falsy, an
error is thrown. However, hosted-git-info may well return a falsy bugs
URL. This can happen when there is no bugs URL in the repo, but
hosted-git-info tries to infer one, but one cannot be inferred, for
example if the repository URL is a SourceHut URL. This is described
here: npm/hosted-git-info#213

For this reason, we should tolerate falsy URLs and fall back to whatever
is defined in `bugs.js`, in this case being sent to
https://www.npmjs.com/package/<packageName>.
@vladh
Copy link
Contributor Author

vladh commented Sep 12, 2023

npm bugs fixed in npm/cli#6798.

vladh added a commit to vladh/cli that referenced this pull request Sep 12, 2023
Currently, if the bugs URL returned from `package.json`, which is
processed through
[hosted-git-info](https://github.com/npm/hosted-git-info), is falsy, an
error is thrown. However, hosted-git-info may well return a falsy bugs
URL. This can happen when there is no bugs URL in the repo, but
hosted-git-info tries to infer one, but one cannot be inferred, for
example if the repository URL is a SourceHut URL. This is described
here: npm/hosted-git-info#213

For this reason, we should tolerate falsy URLs and fall back to whatever
is defined in `bugs.js`, in this case being sent to
https://www.npmjs.com/package/<packageName>.
wraithgar pushed a commit to npm/cli that referenced this pull request Sep 13, 2023
Currently, if the bugs URL returned from `package.json`, which is
processed through
[hosted-git-info](https://github.com/npm/hosted-git-info), is falsy, an
error is thrown. However, hosted-git-info may well return a falsy bugs
URL. This can happen when there is no bugs URL in the repo, but
hosted-git-info tries to infer one, but one cannot be inferred, for
example if the repository URL is a SourceHut URL. This is described
here: npm/hosted-git-info#213

For this reason, we should tolerate falsy URLs and fall back to whatever
is defined in `bugs.js`, in this case being sent to
https://www.npmjs.com/package/<packageName>.
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