setup-purescript icon indicating copy to clipboard operation
setup-purescript copied to clipboard

Not use the versions file for NPM tools

Open pete-murphy opened this issue 4 years ago • 3 comments

Resolves: #23

I've tried to come up with the minimal change to make this work (if I'm understanding the intended behavior). These changes delay the interpretation of VersionFieldVersion until the installMethod is called. So resolve has been updated to return a VersionField instead of Version, and that VersionField is passed through getTool and finally to installMethod where, if it's Latest and the tool is an NPM tool, it is converted to the string "latest" (to generate, e.g., npm install purs-tidy@latest), otherwise it's converted to a version string via Version.showVersion. I'm not sure this is the best way of going about this (or if I've even correctly understood the intended logic), but no other ideas have come to my mind 😅 I'm very much open to suggestion.

pete-murphy avatar Aug 10 '21 03:08 pete-murphy

@ptrfrncsmrph I think this is a nice start! It looks like you're understanding everything correctly. I think we might have to shuffle things around a bit more so that we read the versions file close to where we determine the install method, just so that there are fewer places where we're plumbing through a VersionField that we're going to manipulate later.

We might even need to change a data type like InstallMethod so that it includes an exact version for tarballs (which we resolve from the versions file) and either Latest or Exact versions for NPM-installed tools (which we don't resolve from the versions file).

thomashoneyman avatar Aug 10 '21 18:08 thomashoneyman

Thanks for the feedback! I should be able to give it another pass and re-request review in the next couple days

pete-murphy avatar Aug 11 '21 00:08 pete-murphy

Sorry for delay, I've tried starting over with this, but still not satisfied with the result. What I've got so far: https://github.com/purescript-contrib/setup-purescript/compare/main...ptrfrncsmrph:pm/issue-23-attempt-use-string-as-version. There I'm just passing around the version as a String (which could be literal "latest" or the result of Version.showVersion), which gets awkward when needing to check if that string is greater than or equal to "0.18.0", e.g.

We might even need to change a data type like InstallMethod so that it includes an exact version for tarballs (which we resolve from the versions file) and either Latest or Exact versions for NPM-installed tools (which we don't resolve from the versions file).

Trying to think through how this would work: would the installMethod function be merged into resolve so we have something like

resolve :: Json -> Tool -> ExceptT Error Effect (Maybe InstallMethod)

I've started doing something like that here, but resolve ends up being pretty massive and complex and I might be getting off on the wrong foot: https://github.com/purescript-contrib/setup-purescript/compare/main...ptrfrncsmrph:pm/issue-23-attempt-refactor-install-method.

pete-murphy avatar Aug 23 '21 14:08 pete-murphy