Closed JordanMartinez closed 2 years ago
@thomashoneyman Anything else you see in this PR that is problematic? There might be a few issues still with some future PRs, but this is what the PRs will look like.
I also think if purs-tidy is going to be used, it's very likely that you will want to commit operator files as well, so that the formatting is consistent with the operators defined in the libraries. I don't think you want to rely on whatever the default set is that's shipped with tidy, because it will be inconsistent with any changes you potentially make (like in parsing
).
No, I think this otherwise looks great! đź‘Ť
@natefaubion CI is failing because purs-tidy
doesn't support the type-level integers syntax yet. Could you make a new release?
I also think if purs-tidy is going to be used, it's very likely that you will want to commit operator files as well, so that the formatting is consistent with the operators defined in the libraries. I don't think you want to rely on whatever the default set is that's shipped with tidy, because it will be inconsistent with any changes you potentially make (like in
parsing
).
Last time I tried generating a .tidyoperators
file, the generated file wasn't correct. That's why it's not yet added to the script. I'll need to debug that first before adding that to the script. Otherwise, I'll likely generate an invalid file.
I also think if purs-tidy is going to be used, it's very likely that you will want to commit operator files as well, so that the formatting is consistent with the operators defined in the libraries. I don't think you want to rely on whatever the default set is that's shipped with tidy, because it will be inconsistent with any changes you potentially make (like in
parsing
).Last time I tried generating a
.tidyoperators
file, the generated file wasn't correct. That's why it's not yet added to the script. I'll need to debug that first before adding that to the script. Otherwise, I'll likely generate an invalid file.
Ha... Turns out the .spago
folder didn't exist and that's why the command was failing. spago install
followed by purs-tidy generate-operators $(spago sources)
fixed that.
spago install followed by purs-tidy generate-operators $(spago sources) fixed that.
You should not use $(spago sources)
. You should use xargs like in the README.
spago install followed by purs-tidy generate-operators $(spago sources) fixed that.
You should not use
$(spago sources)
. You should use xargs like in the README.
Since I'm running a PureScript script now, I decided to determine what the directories are and pass these in directly as args to purs-tidy
.
Two questions I currently have about these scripts:
.tidyoperators
file for every repo (if one can be generated)? If so, should CI also generate the file to ensure a change in a dependency causes us to update that file later (e.g. control
's <|>
change wouldn't and how it affects formatting)?spago test
lines that were commented out to get CI to build? Should we uncomment the spago test lines that were commented out to get CI to build?
If they now run properly, then yep!
Should we uncomment the spago test lines that were commented out to get CI to build?
If they now run properly, then yep!
They don't. We're still blocked by spago not having made a new release.
Should we generate a .tidyoperators file for every repo (if one can be generated)? If so, should CI also generate the file to ensure a change in a dependency causes us to update that file later (e.g. control's <|> change wouldn't and how it affects formatting)?
I consider the default operator table as only a convenience for user code. There is no guarantee that what is shipped coincides with what your code is actually using. I personally think that all projects should generate operator files when:
Ok, if our goal is to get 0.15.0 out, I think we need to clarify "what must be done" vs "what we would like to do but can be done later".
What must be done:
What we would like to do but can be done later:
spago test
lines in CIcore
libraries with purs-tidy
Let me clarify:
spago test
doesn't work because we don't have a spago release with the current changes in master
. Even if that was released, we still need https://github.com/purescript/spago/pull/866 to be merged to master
and then released before spago
will work with 0.15.0
. Because pulp test
does work, we can prove that CI passes and uncomment these later. So at the very least, releasing a library does not require the spago blocker to be unblocked, but releasing a library does make progress towards releasing 0.15.0
.purs-tidy
doesn't currently work because it does not support type-level integers. And that itself is problematic for a number of reasons. If we add purs-tidy
to CI, will it block future breaking release updates to core libraries by making PRs fail? Do we somehow disable that only temporarily? What's the logic behind handling that properly? With all of that possible complexity, now is really not the time to do it. Rather, we could do a round of maintenance PRs in the future that formats the code using purs-tidy
, whether it's added to CI or not since most core libraries don't change. Moreover, such PRs don't need to be done in a linear fashion; we could do prelude
and quickcheck
at the same time. All that to say that we can still get formatted code into core
without including it in CI right now.Assuming readers are in agreement with me, the only thing blocking this PR is an agreement that no more compiler PRs are going to be merged. If we still plan to merge work into the compiler (e.g. VTAs PR), then this PR should wait.
Oh, and here's another potential problem. Shouldn't Node be updated to 14 because 12 is almost EOF? https://github.com/purescript/purescript-prelude/blob/master/.github/workflows/ci.yml#L20=
I agree with all the above. And because we aren’t using much in the way of Node features I think we can reasonably move to Node 16. But I’m happy with 14 too.
Ok, the script has been updated to also change Node from 12 to 14 in CI. At this point, we need to decide what compiler PRs will get into 0.15.0 or not. Once those are all merged, we can start releasing here.
@thomashoneyman Can this get an approval?
Description of the change
Backlinking to purescript/purescript#4244. Prepares project for first release that is compatible with PureScript v0.15.0.
:robot: This is an automated pull request to prepare the next release of this library. PR was created via the Release.purs file. Some of the following steps are already done; others should be performed by a human once the pull request is merged:
bower.json
file does not exist.purs-tidy
pulp publish
.