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: Add proper version resolution #25

Merged
merged 3 commits into from
Sep 19, 2024
Merged

Conversation

GearsDatapacks
Copy link
Owner

@GearsDatapacks GearsDatapacks commented Sep 16, 2024

This allows the user to specify package versions using --package-version or -V, and performs version resolution if not specified

This is also fairly hard to test, just like the rest of the I/O stuff. We should probably set up some integration tests at some point, where we have an entire gleam project and we run glemoire in that folder, to test real I/O.

Copy link
Collaborator

@just-maiyak just-maiyak left a comment

Choose a reason for hiding this comment

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

This PR is mostly there, but there are a few bugs still I think :

  • When you do gleamoire gleamoire on an empty cache, it gives you the following error :
gleam run -- gleamoire
   Compiled in 0.04s
    Running gleamoire.main

InterfaceError: Package gleamoire does not contain module gleamoire.
Available modules: 

It's repaired by running gleam clean

  • I just always seem to hit hexdocs with this PR whether I specify a version or not, it never attempts to build the cached PI locally, unless using --refresh

I'm still not sure where the bugs are coming from ... I'm not very free during daytime at the moment, so I cannot do a deep dive right away.
Do you want to investigate the bugs on a call or asynchronously ?

src/gleamoire/version.gleam Show resolved Hide resolved
src/gleamoire/docs.gleam Show resolved Hide resolved
src/gleamoire/version.gleam Show resolved Hide resolved
src/gleamoire/version.gleam Show resolved Hide resolved
src/gleamoire/version.gleam Show resolved Hide resolved
@just-maiyak
Copy link
Collaborator

I'm doing a bit of debuging right now and I determined that, provided I run this in my gleamoire folder, simply passing the query like so

gleam run -- tom

results in resolve_version returning Unresolved whereas it should return Version(1, 0, 1).

packages
|> list.find_map(fn(toml) {
case toml {
tom.Table(dict) ->
Copy link
Collaborator

@just-maiyak just-maiyak Sep 17, 2024

Choose a reason for hiding this comment

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

This is the bug, manifest.toml uses inline tables to denote deps, so it should be :

Suggested change
tom.Table(dict) ->
tom.Table(dict) | tom.InlineTable(dict) ->

We could probably even ommit tom.Table because I don't think gleam uses normal tables for this

Copy link
Owner Author

Choose a reason for hiding this comment

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

Ah that's probably what's causing the bug of never building docs locally

@GearsDatapacks
Copy link
Owner Author

Thanks for the review! I'll go through these as soon as I get time

@GearsDatapacks
Copy link
Owner Author

Looks like we were only cleaning the cache for dependencies and not the main project

image

@just-maiyak
Copy link
Collaborator

I guess we should gleam clean while this is still open. Maybe we can add a // TODO ?

@GearsDatapacks
Copy link
Owner Author

Alright, the two bugs you found have now been fixed, with the exception of gleamoire documenting itself, as discussed on discord.

Copy link
Collaborator

@just-maiyak just-maiyak left a comment

Choose a reason for hiding this comment

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

LGTM ! Amazing work again 🙂

@just-maiyak just-maiyak merged commit 7c82453 into main Sep 19, 2024
1 check passed
@just-maiyak just-maiyak deleted the feat/package-version-flag branch September 19, 2024 06:29
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