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

Remove unsafePerformIO #84

Open
wants to merge 11 commits into
base: master
Choose a base branch
from

Conversation

exarkun
Copy link
Member

@exarkun exarkun commented Jan 18, 2023

It's not clear that these uses are safe and it doesn't seem like much of a hardship to just drop them and force the caller to operate in IO.

@sajith
Copy link
Member

sajith commented Sep 14, 2023

Looks like a nice change, but I'm merely eyeballing. :-)

It would be nice to have this checked by CI, and update documentation with the correct steps for running Haskell tests because simply running runhaskell haskell/test/FECTest.hs as mentioned in the README does not work.

@sajith
Copy link
Member

sajith commented Sep 14, 2023

Well, the README has some notes about installing the library, but it is both outdated (because fec.cabal has a test suite definition now, so cabal test is what should be recommended) and incorrect (because cabal install-ing a library globally based on vague wording is likely to turn out to be a bad idea).

Can we also add some README updates to this PR?

@sajith
Copy link
Member

sajith commented Sep 14, 2023

Incidentally, https://hackage.haskell.org/package/fec is bothersome too. The package there is outdated, and there's just one maintainer. I'm guessing that it doesn't bother Nix people so much. :-)

(Also, the Haskell package probably should be called zfec to be more accurate, not fec? But then the module is Codec.FEC...)

@exarkun
Copy link
Member Author

exarkun commented Sep 14, 2023

Looks like a nice change, but I'm merely eyeballing. :-)

It would be nice to have this checked by CI, and update documentation with the correct steps for running Haskell tests because simply running runhaskell haskell/test/FECTest.hs as mentioned in the README does not work.

I think those are good ideas. Since I seem to have discovered the cause of the corrupt results (#89 / #91) I don't know if I'll push forward this change much more in the near future but I will try to put some CI / documentation improvements into another PR.

@exarkun
Copy link
Member Author

exarkun commented Sep 14, 2023

Looks like a nice change, but I'm merely eyeballing. :-)

It would be nice to have this checked by CI, and update documentation with the correct steps for running Haskell tests because simply running runhaskell haskell/test/FECTest.hs as mentioned in the README does not work.

Ah - CI does run the Haskell tests already, eg https://app.circleci.com/pipelines/github/tahoe-lafs/zfec/5/workflows/e1f141a4-cc88-49eb-aafb-f43d76171448/jobs/5

@sajith
Copy link
Member

sajith commented Sep 14, 2023

Ah, I wasn't paying attention. It doesn't seem to run cabal test though, or does it?

@sajith
Copy link
Member

sajith commented Sep 14, 2023

Also, it doesn't seem that CircleCI status checks appear on GitHub PRs?

@exarkun
Copy link
Member Author

exarkun commented Sep 14, 2023

Ah, I wasn't paying attention. It doesn't seem to run cabal test though, or does it?

Indirectly it does - but it's true that it drives that process with nix build. (This builds the library and, perhaps surprisingly, runs the test suite.

Also, it doesn't seem that CircleCI status checks appear on GitHub PRs?

They do but it looks like CircleCI was actually set up after this PR was created so it didn't show up on this PR's CI runs.

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.

2 participants