-
Notifications
You must be signed in to change notification settings - Fork 53
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
Ease consumption by means of .tool-versions #159
Ease consumption by means of .tool-versions #159
Conversation
@@ -16,9 +16,11 @@ workflow by: | |||
- optionally, installing [Gleam](https://gleam.run/) | |||
- optionally, installing [`rebar3`](https://www.rebar3.org/) | |||
- optionally, installing [`hex`](https://hex.pm/) | |||
- optionally, opting for strict or loose version matching |
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.
Not specific to this pull request, but was missing.
This value is check by code, in any case
@@ -11,7 +11,6 @@ inputs: | |||
otp-version: | |||
description: Version range or exact version of Erlang/OTP to use, | |||
or false when installing only Gleam without OTP | |||
required: true |
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.
This is checked by code, in any case
It appears we can't install Elixir twice on Windows, but can on Ubuntu; this is outside the scope of this branch's context so I won't fix it now
Moving to Review-ready. |
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.
I'm personally not a fan of projects that commit .tool-versions
as they would overwrite my global Elixir version choice. We wouldn't be able to easily support a valid asdf config like: elixir: 1.14.2-otp-25
. I'm personally -1 on adding this but I don't have a strong opinion. For people who are checking in the versions file, it is not easy to parse and fill in the setup-beam version values in the workflow, so this change is definitely very convenient.
version-file: __tests__/.tool-versions | ||
version-type: strict |
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.
I understand the rationale but I think it would be more convenient if we didn't have to specify version-type if version-file is set. (we'd fail if it is set to something else than strict though.) Not a strong opinion.
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.
Are there actually cases where people put their .tool-versions
deeper in their project? I think convention over configuration would indeed be better here, and then you provide a boolean. versions-from-file: true
or something like that.
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.
version-type
is loose
by default, so not specifying it is the same as choosing non-strict. I guess validating, in that case, unless it is strict
, would always fail (?).
There is, however, a note on the README for this, so it'll fail at most once on your first run, where you include the new option. This seems safe.
@starbelly, as for a new option alongside a new option, I don't understand the rationale.
Are there actually cases where people put their .tool-versions deeper in their project?
It wouldn't matter, because the action reads a path, so you can put it wherever you want. The fact I named it like that, taking into account the issue that gave origin to this pull request, is that we might support more conventions in the future. If we think we won't, we should just change the option name to parse-tool-versions
and use a default as you propose.
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.
@paulo-ferraz-oliveira Eh, it's not really a big deal IMO. I think the way you have it is more flexible, what I just suggested would result in almost the same amount of typing 😁
I'm with you here, I see this as mostly useful for private applications myself. Not too many libs out in the wild using |
From what you wrote, I have a feeling it doesn't hurt, but none of you are inclined to accept it, is that it, @starbelly, @wojtekmach? I guess if that's the case we need a better discussion workflow. 😄 There was an issue around this, and the idea wasn't reject by anyone. I later wrote I'd start to implement it, and I was apparently encouraged, but I might have over-read it, or under-reading what you wrote here. I don't mind closing this pull request and moving forward, but I would like more concrete opinions on whether we keep it or drop it. 👍 |
I think having |
Sorry I wasn't clear. I personally wouldn't add it for the reason mentioned. But I'm not opposed to adding it. Does that make sense? |
It does, thanks :) |
And I know there might be this tendency to say "but it's optional", etc., but options have to be maintained too, so I do understand the rationale behind your rationale. (and also that we might run into issues regarding proper parsing of the tools file). |
I just wanted to say great job on this @paulo-ferraz-oliveira , there's a lot of teams that will get a lot of mileage out of it! ❤️ |
This allows using a
.tool-versions
file as perasdf
constraints.The
README.md
changes should make the content of the pull request explicit.Edit
This is working as expected as can be seen for example repo (link to
.tool-versions
):.tool-versions
,version-type =/= strict
Closes #137.