Add support for short repository property and make it default #83
Conversation
| const githubMatch = (this.mergedPackageData.repository.url || this.mergedPackageData.homepage).match(/github\.com\/(.+?)(?:\.git|\/)?$/) | ||
| let repo = this.mergedPackageData.repository || null | ||
| const githubMatch = (this.mergedPackageData.repository.url || this.mergedPackageData.homepage).match(/github\.com\/(.+?)(?:\.git|\/)?$/) || | ||
| (this.mergedPackageData.repository.match(/^(.+\/.+)$/) && !this.mergedPackageData.repository.match(/^(.+:.+\/.+)$/)) |
There was a problem hiding this comment.
Couldn't do this the other way. If you have a better idea how to match user/repo but not (bitbucket|gist|gitlab):user/repo, please tell me.
There was a problem hiding this comment.
This should do it:
const githubMatch = this.mergedPackageData.repository.match(/^(?:github:)?(.+\/.+)$/)
|| (this.mergedPackageData.repository.url || this.mergedPackageData.homepage).match(/github\.com\/(.+?)(?:\.git|\/)?$/The ?: means non capturing group, so whatever is in the ?: group doesn't affect the match.
There was a problem hiding this comment.
It looks like that doesn't work. Basically, all it has to do is to ignore repository names which contain : (e.g. bitbucket:user/repo). I've checked it, creating GitHub reposities with : in it is impossible, they're automatically converted to -, so the : check should work fine for the purpose.
There was a problem hiding this comment.
Okay, I'll clone and take a look 👍
There was a problem hiding this comment.
const githubMatch = this.mergedPackageData.repository.match(/^(?:github:)?([^\/]+\/[^\/]+)$/) ||
(this.mergedPackageData.repository.url || this.mergedPackageData.homepage).match(/github\.com\/(.+?)(?:\.git|\/)?$/)Seems to do the trick. Can you verify?
There was a problem hiding this comment.
Actually, I think I understand what you mean now. Leave it with me. Will merge it in and adjust.
| const githubMatch = (this.mergedPackageData.repository.url || this.mergedPackageData.homepage).match(/github\.com\/(.+?)(?:\.git|\/)?$/) | ||
| let repo = this.mergedPackageData.repository || null | ||
| const githubMatch = (this.mergedPackageData.repository.url || this.mergedPackageData.homepage).match(/github\.com\/(.+?)(?:\.git|\/)?$/) || | ||
| (this.mergedPackageData.repository.match(/^(.+\/.+)$/) && !this.mergedPackageData.repository.match(/^(.+:.+\/.+)$/)) |
There was a problem hiding this comment.
This should do it:
const githubMatch = this.mergedPackageData.repository.match(/^(?:github:)?(.+\/.+)$/)
|| (this.mergedPackageData.repository.url || this.mergedPackageData.homepage).match(/github\.com\/(.+?)(?:\.git|\/)?$/The ?: means non capturing group, so whatever is in the ?: group doesn't affect the match.
| type: 'git', | ||
| url: `https://github.com/${githubSlug}.git` | ||
| }, | ||
| repository: `${githubSlug}`, |
There was a problem hiding this comment.
repository: githubSlug, is fine here
|
Released to v1.2.0 👏 |
|
My only concern with this is if older versions of npm doesn't support this, however I would assume it is not important for npm and node at all, so I'm not fussed as would be a non-issue |
|
I remember using this a few years ago, back in the time of node 0.x. I guess it isn't a problem for older versions. |
|
Even better! |
As requested in bevry/editions#16. Please review this to make sure there are no issues. About the README changes, I just ran the compile script (
npm run-script compile)./cc @balupton