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

add build_spec module, not yet used in match_spec #340

Merged
merged 7 commits into from
Sep 20, 2023

Conversation

YeungOnion
Copy link
Contributor

this creates the module to define and parse the build number spec

Next PR will use the parsing and matching in crate::match_spec

@YeungOnion YeungOnion marked this pull request as draft September 16, 2023 00:20
@YeungOnion
Copy link
Contributor Author

Whoops, forgot to make a PR for the other two changes. I'll separate those out and then mark this as ready.

  • add pixi task for cargo to expose cargo
  • seemed like version_spec::constraint::Constraint shouldn't be explicitly used outside of the version_spec module since we just parse as a whole VersionSpec and call VersionSpec::matches

@YeungOnion YeungOnion force-pushed the main branch 2 times, most recently from 8c5dcb1 to 2a2837c Compare September 16, 2023 01:22
@baszalmstra
Copy link
Collaborator

Nice work @YeungOnion ! I merged the first two PRs

@YeungOnion
Copy link
Contributor Author

YeungOnion commented Sep 16, 2023

One of these CI failures is from not calling build_spec::BuildNumberSpec::matches. I imagine that's an issue as it's not part of an API defined elsewhere so not being used means it's useless.

The only way to call it as is, would be to make all the changes in the match_spec module and that touches a few different submodules.

An alternative to pass the check is to define API at crate level and use that API in this module. All types with Spec at end of their name have matches methods. Perhaps a trait that all ...Spec would impl would be a good fit for API,

trait Matchable {
    type Record; // very open to better names
    fn matches(x: &Record) -> bool;
}

All the ...Spec types also parse from strings, but that's uncoupled.

I like the second approach, but I don't see real value beyond uncoupling the PR for build number specs into one for module and the other for usage since there aren't other types yet to create that would impl Matchable, are there? I'd just expect the ...Specs to use them. What are your thoughts on this?

Adds some formatting improvements for solvables and out logging.

Also adds:
- Additional tests
- Reformats some of the insta tests to make them more clear (e.g. `9`
becomes `foo=9`).
@tdejager
Copy link
Collaborator

tdejager commented Sep 18, 2023

I think either removing it if it's unused. Or what I'm thinking you are saying is to actually change methods to use this function is a good idea.

The other alternative might be a good idea, but I agree it is out of context of this PR :)

But Ill let @baszalmstra chime in here.

Adds a generic bounded/unbounded `Range` (taken from pubgrub,
contributed there by me) struct which can be used to easily craft
`VersionSet` implementations.

I also moved files to the `internal` folder for types that should only
be used inside the crate.

---------

Co-authored-by: Tim de Jager <tim@prefix.dev>
@baszalmstra
Copy link
Collaborator

IMO we add a #[allow(unused)] with a comment that explains why it is there and merge it. I assume the next PR will start using this method? We can then immediately remove it.

I do like the Matchable idea but maybe for another PR?

@YeungOnion YeungOnion marked this pull request as ready for review September 20, 2023 03:17
@YeungOnion
Copy link
Contributor Author

Think that'll work well. I opened the other PR about a trait providing a matches method as #345

@baszalmstra baszalmstra merged commit 51e959b into conda:main Sep 20, 2023
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