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

add Boolean kind to Prim.Boolean #3389

Merged
merged 1 commit into from Nov 25, 2018

Conversation

justinwoo
Copy link
Contributor

Adds Boolean kind and True/False types of Boolean to Prim.Boolean. Related to #3383

Corresponding typelevel-prelude change: justinwoo/purescript-typelevel-prelude@b82137f

@justinwoo
Copy link
Contributor Author

Builds timed out. Could someone re-run them if that's the issue?

pattern SymbolContains = Qualified (Just PrimSymbol) (ProperName "Contains")

pattern SymbolBreakOn :: Qualified (ProperName 'ClassName)
pattern SymbolBreakOn = Qualified (Just PrimSymbol) (ProperName "BreakOn")
Copy link
Member

Choose a reason for hiding this comment

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

Did you mean to add these in this PR?

Copy link
Contributor Author

@justinwoo justinwoo Jul 11, 2018

Choose a reason for hiding this comment

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

oh, sorry, applied a patch but it was wrong 😅, i'll try to get around to updating it later

@LiamGoodacre
Copy link
Member

LiamGoodacre commented Jul 11, 2018

I restarted the timed out build.

@justinwoo
Copy link
Contributor Author

4/5 ran within time this time through...

@joneshf
Copy link
Member

joneshf commented Jul 11, 2018

If you need to restart the build and don't have access to it on Travis, you can close this PR and open it again. It will restart the build.

I think it means all builds will run again, but it beats waiting for someone else to come along.

@hdgarrood
Copy link
Contributor

No, please don’t do that - instead push an empty commit (as the CI log should suggest). As far as I’m aware that’s the only way to get travis to reuse the progress it made during its previous attempt.

@garyb
Copy link
Member

garyb commented Jul 11, 2018

I've restarted the failing job a couple of times fwiw 😕

@hdgarrood
Copy link
Contributor

Restarting jobs which have timed out is generally not very effective in my experience; we should be encouraging the submitter to push empty commits instead.

@LiamGoodacre
Copy link
Member

(Instead of empty commits you can regenerate your existing commit with git commit --amend --no-edit - this new commit will have a different commit hash. Then force push.)

@joneshf
Copy link
Member

joneshf commented Jul 11, 2018

An alternate to any of our suggestions could be focusing on making CI more reliable 😄.

@hdgarrood
Copy link
Contributor

Amending and force pushing doesn’t work either. I think Travis must take the cache from the parent commit each time.

Incidentally I spent quite a lot of time on this script and I’m fairly confident it’s the best we can do without moving to a different CI platform.

@justinwoo
Copy link
Contributor Author

The previous amend was to address some mistaken code being added, let's see an empty commit

@hdgarrood
Copy link
Contributor

I’m fairly confident it’s the best we can do without moving to a different CI platform.

Just to expand on this a little: I will accept without reservation that this is a pretty sorry state of affairs. I would love to be proven wrong on the above but unfortunately I really do think it’s very unlikely.

@joneshf
Copy link
Member

joneshf commented Jul 14, 2018

Looks like we build the haddocks for dependencies of every job in the matrix whether it has BUILD_TYPE=haddock or not. The job that's failing doesn't have BUILD_TYPE=haddock. Given that it's timing out when uploading the cache–almost done with the build–that extra time with the haddocks is probably enough to push it over the limit. Should we change the jobs to only build haddocks when necessary?

Is it this line?

$TIMEOUT 45m $STACK --install-ghc build \
--only-dependencies --test --haddock \
|| ret=$?

I'm more than willing to submit a PR for it.

@nwolverson
Copy link
Contributor

@joneshf I applied that suggestion in my fork here purerl@5b656c8 - not quite sure if it helped or I just had stuff cached by this point, but I think by my understanding this should be right - I believe the cache will be separate for the different entries in our build matrix

@hdgarrood
Copy link
Contributor

Sorry for the delay in responding. Good spot - yes I think that would help.

@justinwoo
Copy link
Contributor Author

Heyy, this passed now :)

@@ -329,6 +343,21 @@ partial = primClass "Partial" $ T.unlines
, "[the Partial type class guide](https://github.com/purescript/documentation/blob/master/guides/The-Partial-type-class.md)."
]

kindBoolean :: Declaration
kindBoolean = primKindOf (P.primSubName "Boolean") "Boolean" $ T.unlines
[ "The `Boolean` kind provides a the type-level True/False types"
Copy link
Member

Choose a reason for hiding this comment

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

Typo a the.

@justinwoo justinwoo force-pushed the prim-boolean branch 2 times, most recently from 9d67c5f to 599a0c4 Compare August 11, 2018 14:00
@justinwoo
Copy link
Contributor Author

Cleaned up now. I can rebase #3383 later, and I think I might implement Row.Contains also since sometimes I've needed that and it's similar to the old RowLacks that was expensive as a library

@LiamGoodacre
Copy link
Member

Some of the BreakOn and Contains stuff is still in this PR, @justinwoo ?

@justinwoo
Copy link
Contributor Author

Finally removed it

Copy link
Member

@LiamGoodacre LiamGoodacre left a comment

Choose a reason for hiding this comment

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

Looks great, thanks!

@justinwoo justinwoo mentioned this pull request Aug 12, 2018
@LiamGoodacre LiamGoodacre merged commit dcdd70d into purescript:master Nov 25, 2018
@garyb garyb mentioned this pull request Jan 12, 2019
3 tasks
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.

None yet

6 participants