-
Notifications
You must be signed in to change notification settings - Fork 71
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
Support for weeder step #742
base: master
Are you sure you want to change the base?
Conversation
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.
Before I even look at this PR more closely. It contains independent changes like ghExprWrap
and changes elsewhere. Do them separately. I don't know whether I like them or not.
Ok, I have removed the independent changes from this branch. |
let ifCond = ghCompilerVersionArithPredicate allVersions range in | ||
for_ pkgs $ \Pkg{pkgName} -> do | ||
githubUsesIf "weeder" "freckle/weeder-action@v2" ifCond $ buildList $ do | ||
item ("ghc-version", ghWrapExpr "matrix.compilerVersion") |
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 don't see this working. Comparing strings like 8.10.7
and 8.8.4
with operators like >=
wouldn't work as intended.
Add test
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.
Comparing strings like
8.10.7
and8.8.4
with operators like>=
wouldn't work as intended.
The strings get converted to numeric representation before comparison. E.g., with this config:
weeder: >= 9.4 && < 9.8
the resulting comparison is:
${{ env.HCNUMVER >= 90400 && env.HCNUMVER < 90800 }}
And here's a live demonstration featuring coomparison against a number like 9.10.7
.
I have also now added a golden test.
test/Tests.hs
Outdated
@@ -37,44 +39,67 @@ main = do | |||
, fixtureGoldenTest "copy-fields-some" | |||
, fixtureGoldenTest "copy-fields-none" | |||
] | |||
, testGroup "github-specific" |
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.
unnecessary complication.
Please, do the least required stuff. I'd tell if it's not enough, now it's too much.
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.
Do you mean, remove the extra testGroup "github-specific"
layer, but keep fixtureWeederGoldenTest
? I've removed the extra group layer now.
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.
Or if you mean that fixtureWeederGoldenTest
is too complicated, could you help give an idea of what a minimal test would look like?
Closes #132
In this PR I also parameterized the
freeToArith
function to allow for formatting an expression according to GitHub actions expression syntax.Test procedure
Example output: swarm-game/swarm#2059