-
Notifications
You must be signed in to change notification settings - Fork 48
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 parsing of some purl types. Fix tests. #46
Conversation
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Just a general remark from someone who's not into Go, but trying to help out with a review to get things rolling again: What's making the review hard for me is that most of the commit messages do not provide a rationale why you're doing what you're doing, but you just repeat in prose what's more or less obvious from the diff.
testdata/test-suite-data.json
Outdated
@@ -498,7 +498,7 @@ | |||
"type": "huggingface", | |||
"namespace": "EleutherAI", | |||
"name": "gpt-neo-1.3B", | |||
"version": "797174552ae47f449ab70b684cabcb6603e5e85e", | |||
"version": "797174552AE47F449AB70B684CABCB6603E5E85E", |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
For consistency with other test data in this file, wouldn't it make more sense to lower-case the SHA1 in the purl in line 496 instead?
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Ok, probably not that important as you're changing it back in a later commit anyway.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Yes I changed it back because that was a mistake as a huggingface version must be lowercase https://github.com/package-url/purl-spec/blob/master/PURL-TYPES.rst#huggingface
I'd appreciate a review from @shibumi or @pombredanne. |
Sorry, when I wrote that in the commit messages, I meant that the logic must be that way because of what the spec states.
|
Thanks! Would you mind amending the commit messages with these, and force-pushing the branch for this PR again? |
…ata during the test action. This was confusing as there were multiple sources of truth for the test data, one of which was not used. There is only one now.
…sitory, as specified in the spec https://github.com/package-url/purl-spec/blob/master/PURL-TYPES.rst#mlflow Also fix the qualifier order in the mlflow test to match the example in the spec.
Done |
Thanks again. I'm not sure about the rules in this repo, but usually the Git commit message title is limited to something like 72 chars (and shouldn't contain a trailing dot; think of it like the title for a book), and the "real" explanation should go as a separate paragraph in the body of the commit message. Anyway, if there's no negative response from @shibumi or @pombredanne by today, I plan to merge this to move things forward. |
Ok, let's just merge this to move things forward. |
[![Mend Renovate](https://app.renovatebot.com/images/banner.svg)](https://renovatebot.com) This PR contains the following updates: | Package | Type | Update | Change | |---|---|---|---| | [github.com/package-url/packageurl-go](https://github.com/package-url/packageurl-go) | require | patch | `v0.1.0` -> `v0.1.1` | | [github.com/urfave/cli/v2](https://github.com/urfave/cli) | require | patch | `v2.25.6` -> `v2.25.7` | | golang.org/x/tools | require | minor | `v0.9.3` -> `v0.10.0` | --- ### Release Notes <details> <summary>package-url/packageurl-go</summary> ### [`v0.1.1`](https://github.com/package-url/packageurl-go/releases/tag/v0.1.1) [Compare Source](https://github.com/package-url/packageurl-go/compare/v0.1.0...v0.1.1) ### Changes - [make PackageURL.String() use a value receiver rather than pointer rec…](https://github.com/package-url/packageurl-go/commit/0789fa562603bb38a897134d735e7fc7ddf44b1e) - [fix: update Go and CI / tests](https://github.com/package-url/packageurl-go/commit/77b4cfeee1c804b53caebca3106b1fe7c2c20d9a) - [fix: linting issues](https://github.com/package-url/packageurl-go/commit/52157000a8d68a40397856d8c2128680cc11e1a9) - [fix: empty slice declaration using literal](https://github.com/package-url/packageurl-go/commit/7711a9e1356d3a2ee69f9b8a85d05a91ca0e544e) - [fix: remove travis.yml](https://github.com/package-url/packageurl-go/commit/36e2941dc9960a206504bb45e881717411d7adbd) - [fix: implement missing Types: cran, conan, swift](https://github.com/package-url/packageurl-go/commit/2101d64fcfee22dc344a0887a6793483c1cda33e) - [docs: add badges for README.md](https://github.com/package-url/packageurl-go/commit/f3dc3162680403e1cc638b6f60736c52f29c46a3) - [fix: reformat code + doc strings](https://github.com/package-url/packageurl-go/commit/3245c8ba61e712111c45b4d14b80ec58334311a0) - [fix: spelling in comments](https://github.com/package-url/packageurl-go/commit/07a9f5a63080dec6a80c69c484ad9667e70d0069) - [fix: rename debian type](https://github.com/package-url/packageurl-go/commit/5cd1dfb82b3add4880819caf25e5199bd43fc893) - [feat: add cargo custom type](https://github.com/package-url/packageurl-go/commit/fdd45fab1ae3d53240e6f8b7d5a62a42f6a30296) - [feat: add missing package urls](https://github.com/package-url/packageurl-go/commit/f395a911086ba1325189bc84fbe7aa9f500f1091) - [fix: issue](https://github.com/package-url/packageurl-go/commit/0ccaa1214f2b4e02f6b239b52fd1f4b1e553a297) [https://github.com/package-url/packageurl-go/issues/20](https://github.com/package-url/packageurl-go/issues/20) [license detection](https://github.com/package-url/packageurl-go/commit/0ccaa1214f2b4e02f6b239b52fd1f4b1e553a297) - [fix: add license symlink](https://github.com/package-url/packageurl-go/commit/d70459300c8aa972fa61818f141a7b52ad0e5c9b) - [Add in test-suite-json, again](https://github.com/package-url/packageurl-go/commit/ab67608befffc606e03a3d23772deb3eb4fec71d) - [Add in test-suite-json, again](https://github.com/package-url/packageurl-go/commit/ab67608befffc606e03a3d23772deb3eb4fec71d) - [Tests fixed](https://github.com/package-url/packageurl-go/commit/32f04a9a10bf8781eb824460e070872b5afb5836) - [Merge pull request](https://github.com/package-url/packageurl-go/commit/89078438f1709503264de381344c07acebbbf901) [https://github.com/package-url/packageurl-go/pull/32](https://github.com/package-url/packageurl-go/pull/32) [from DarthHater/TestSuiteData](https://github.com/package-url/packageurl-go/commit/89078438f1709503264de381344c07acebbbf901) - [Copy the latest test data from master and stop downloading the test d…](https://github.com/package-url/packageurl-go/commit/1f567e34d43b2727fe73403ef0cc246c866f5b85) - [Fix huggingface version parsing. The version must be lowercase as spe…](https://github.com/package-url/packageurl-go/commit/69d3c91e26332a2e67812d391ec0361e09f64b00) - [Fix mlflow name adjustments. The case of the name depends on the repo…](https://github.com/package-url/packageurl-go/commit/7e18e09ad38cc57b855a5234bd25c360a7cba0f2) - [Fix composer name and namespace parsing. They must be lowercase, as s…](https://github.com/package-url/packageurl-go/commit/2e2d03f9309372895124aa7a4220c190bbd0ae2b) - [Merge pull request](https://github.com/package-url/packageurl-go/commit/358f10025d4a923f7f9babc766f0968720e82240) [https://github.com/package-url/packageurl-go/pull/46](https://github.com/package-url/packageurl-go/pull/46) [from Octogonapus/fix_tests](https://github.com/package-url/packageurl-go/commit/358f10025d4a923f7f9babc766f0968720e82240) </details> <details> <summary>urfave/cli</summary> ### [`v2.25.7`](https://github.com/urfave/cli/releases/tag/v2.25.7) [Compare Source](https://github.com/urfave/cli/compare/v2.25.6...v2.25.7) #### What's Changed - Fix: fix v2 broken tests by [@​dearchap](https://github.com/dearchap) in [https://github.com/urfave/cli/pull/1757](https://github.com/urfave/cli/pull/1757) - Fix:(issue\_1755) Ensure that timestamp flag destination is set correctly by [@​dearchap](https://github.com/dearchap) in [https://github.com/urfave/cli/pull/1756](https://github.com/urfave/cli/pull/1756) **Full Changelog**: urfave/cli@v2.25.6...v2.25.7 </details> --- ### Configuration 📅 **Schedule**: Branch creation - "before 6am on monday" in timezone Australia/Sydney, Automerge - At any time (no schedule defined). 🚦 **Automerge**: Disabled by config. Please merge this manually once you are satisfied. ♻ **Rebasing**: Whenever PR becomes conflicted, or you tick the rebase/retry checkbox. 👻 **Immortal**: This PR will be recreated if closed unmerged. Get [config help](https://github.com/renovatebot/renovate/discussions) if that's undesired. --- - [ ] <!-- rebase-check -->If you want to rebase/retry this PR, check this box --- This PR has been generated by [Mend Renovate](https://www.mend.io/free-developer-tools/renovate/). View repository job log [here](https://developer.mend.io/github/google/osv-scanner). <!--renovate-debug:eyJjcmVhdGVkSW5WZXIiOiIzNS4xMjYuMCIsInVwZGF0ZWRJblZlciI6IjM1LjEzMS4wIiwidGFyZ2V0QnJhbmNoIjoibWFpbiJ9-->
This PR adds the latest test data from master into
testdata/
instead of keeping an old version around.This PR also stops overwriting that test data during test execution because that is confusing when the test data is already in the source tree.
Fixes I did while reading the purl spec:
Closes #38.