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

feat(usePantry): Make versions take (PlainObject | string) | (PlainObject | string)[]. #192

Merged
merged 7 commits into from
Oct 24, 2023

Conversation

rustdevbtw
Copy link
Contributor

@rustdevbtw rustdevbtw commented Oct 24, 2023

Makes the versions key to support a combination of PlainObject and string OR an Array of type Array<PlainObject | string>.

For example:

versions:
    - github: org/repo/releases
       strip: /v/
    - github: org/repo/tags
       strip: /v/

As well as:

versions:
    - github: org/repo/releases
       strip: /v/
    - github: org/repo/tags # Some version listed in tags, but without a proper release.
       strip: /v/
    - gitlab: org/repo/releases # Some version listed only in some GitLab repo releases
       strip: /v/
    - "1.0.0" # Some version listed nowhere, but exists

This all, while keeping full backwards compatibility! (hopefully)

@mxcl
Copy link
Member

mxcl commented Oct 24, 2023

Seems like a solid addition with no trade-offs I can think of, but maybe @jhheider can imagine some

@rustdevbtw
Copy link
Contributor Author

Seems like the PIP CI is having issues (definitely not related to it, because it only applies to pkg build, at the start, and the error occurs at a later part).

@mxcl
Copy link
Member

mxcl commented Oct 24, 2023

yeah pip has had problems every build recently. We should probably pin all these builds to known buildable versions

@rustdevbtw
Copy link
Contributor Author

@mxcl Except the PIP one, every other test seems to pass.

@mxcl
Copy link
Member

mxcl commented Oct 24, 2023

Yep we can merge if @jhheider agrees with me that this seems safe and good

@rustdevbtw
Copy link
Contributor Author

@mxcl I added additional tests (to confirm it works) for usePantry.getVersions.

Copy link
Contributor

@jhheider jhheider left a comment

Choose a reason for hiding this comment

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

The code change looks good. We should try to keep style changes out of logic changes.

lib/usePantry.getVersions.ts Outdated Show resolved Hide resolved
lib/usePantry.getVersions.ts Outdated Show resolved Hide resolved
@jhheider
Copy link
Contributor

Yep we can merge if @jhheider agrees with me that this seems safe and good

Yeah, we discussed at length on discord. I think this gives us some great options without altering existing parsing.

@rustdevbtw
Copy link
Contributor Author

I've applied these changes. As for style stuff, I've Deno autoformatting enabled, so it formatted that.

@rustdevbtw
Copy link
Contributor Author

Anything else?

@jhheider
Copy link
Contributor

If mxcl is ok with all the autoformating, I'm ok with the logic.

@rustdevbtw
Copy link
Contributor Author

Oh no, I didn't see the Deno permissions. I think I've to remove that test for now.

@jhheider
Copy link
Contributor

Oh no, I didn't see the Deno permissions. I think I've to remove that test for now.

Can we add the permission we need? Better to test than not.

@rustdevbtw
Copy link
Contributor Author

Oh no, I didn't see the Deno permissions. I think I've to remove that test for now.

Can we add the permission we need? Better to test than not.

Just added that

@rustdevbtw
Copy link
Contributor Author

Seems like permissions aren't just enough. Doing github version checks require GH_TOKEN as well.

@rustdevbtw rustdevbtw changed the title feat(usePantry): Make versions to be (PlainObject | string) | (PlainObject | string)[]. feat(usePantry): Make versions take (PlainObject | string) | (PlainObject | string)[]. Oct 24, 2023
@rustdevbtw
Copy link
Contributor Author

@jhheider Can you try again please?

@mxcl
Copy link
Member

mxcl commented Oct 24, 2023

If mxcl is ok with all the autoformating, I'm ok with the logic.

It's not cool, but we'll live. In general, in open source, you adopt the project’s styling, you don’t impose your own.

@rustdevbtw
Copy link
Contributor Author

If mxcl is ok with all the autoformating, I'm ok with the logic.

It's not cool, but we'll live. In general, in open source, you adopt the project’s styling, you don’t impose your own.

I actually didn't even know that I had autoformatting enabled before @jhheider mentioned the styling

@jhheider jhheider merged commit 936da0a into pkgxdev:main Oct 24, 2023
9 of 10 checks passed
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.

3 participants