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

create purescript-nix-buitins library #40

Open
cdepillabout opened this issue Jan 18, 2022 · 9 comments
Open

create purescript-nix-buitins library #40

cdepillabout opened this issue Jan 18, 2022 · 9 comments
Labels
enhancement New feature or request good first issue Good for newcomers help wanted Extra attention is needed

Comments

@cdepillabout
Copy link
Member

cdepillabout commented Jan 18, 2022

It would be nice to have a library like purescript-nix-builtins that provides FFI for all the different Nix builtins (like builtins.deepSeq, builtins.getEnv, builtins.mapAttrs, etc). This library should also provide FFI data types for types from Nix that are not available from PureScript by default. For instance, a Path type to represent Nix paths.

A couple other points:

@cdepillabout cdepillabout added enhancement New feature or request good first issue Good for newcomers help wanted Extra attention is needed labels Jan 18, 2022
@m-bock
Copy link

m-bock commented Jan 19, 2022

great idea. Regarding the separated "types" package: would it maybe make sense to split this up into sub packages, in the purescript style of having one repo around one core data type. on the JS PS side there are the purescript-foreign-* packages.

For purenix I think the following would make sense

the linked repos contain stub implementations that I needed in a feature branch of miraculix. They follow the idea to be as close as possible to the JS versions, except the 'path' one naturally.

What do you think about it? I think it does not contradict with the plan to have purescript-nix-builintins. purescript-nix-builintins-types would be either obsolete or could maybe just re-export from the foreign packages.

@cdepillabout
Copy link
Member Author

cdepillabout commented Jan 20, 2022

That's a good idea. I agree that it does make sense to have purescript-foreign-path and purescript-foreign-object (or as you say, probably purescript-foreign-attrset).

With those libraries available, I agree it might not make sense to have purescript-nix-types. Although I think it still would be nice if purescript-nix-builtins exported everything from purescript-foreign-path and purescript-foreign-object. Or possibly have a single library called purescript-nix that exports everything from purescript-nix-builtins, purescript-foreign-path, and purescript-foreign-object.

Another comment: In cabal2nixWithoutIFD, it seemed helpful to define a fully opaque AttrSet type, like this:

https://github.com/cdepillabout/cabal2nixWithoutIFD/blob/484515bdec2ccf9dfc02b9a442b801bc2d17b9cc/purescript-parser-combinator/src/NixBuiltins.purs#L6-L8

This is in contrast to PureScript's Foreign.Object, which is effectively a Map String a (the values in the object are all the same type):

https://pursuit.purescript.org/packages/purescript-foreign-object/3.0.0/docs/Foreign.Object

In practice, AttrSet would be isomorphic to Object Foreign, so it is not completely necessary, but probably easier to beginners to figure out.


If someone wants to get started working on this, please let me know and I can create you some repos under the PureNix Organization to get started with.

@m-bock
Copy link

m-bock commented Jan 21, 2022

yeah. I'd be up for starting with this. Think I'd like to start with purescript-foreign. Would be nice if you create a repo under the PureNix org.

@jonascarpay
Copy link
Member

I went ahead and created https://github.com/purenix-org/purescript-foreign, you should have an admin invite

@m-bock
Copy link

m-bock commented Jan 22, 2022

@jonascarpay

Now I adjusted the bits that I already had to a version that I think is ready to be used. It's my first purenix port. So I'll list a couple of concepts that you'll find in the repo:

  • Splits up a spago.dhall into spago-test.dhall and spago-lib.dhall. The actual spago.dhall file is kept, mainly for IDE's to work properly

  • Uses miraculix-lite as a testing framework. That's a version of miraculix that bundles as many as possible of its own dependencies.

  • CI: Run build and test on GH actions

  • Docs: since we don't have a purenix pursuit yet. Docs are published to GH pages

  • Not everything can be ported yet, the README lists the dependencies that this port is waiting for to be ported. Unported code is intentionally kept in the codebase in comments.

If you have any feedback about those workflows, feel free to leave a comment!

@cdepillabout
Copy link
Member Author

@thought2 I bumped you up from an outside contributor to an owner of the PureNix organization. It looks like you'll have to either check your email or go to https://github.com/purenix-org to accept the invitation.

We put together a document for joining the PureNix organization. Please take a look: https://github.com/purenix-org/purenix/blob/main/.github/MEMBERS.md.


Splits up a spago.dhall into spago-test.dhall and spago-lib.dhall. The actual spago.dhall file is kept, mainly for IDE's to work properly

Is this mainly for being able to run the tests? I'm guessing we'd hopefully be able to go back to just a single spago.dhall once #39 is implemented?

tests

The tests are really nice! This is exactly the kind of thing I was imagining!

the README lists the dependencies that this port is waiting for to be ported. Unported code is intentionally kept in the codebase in comments.

Thanks for documenting this, and keeping the unported code in the codebase (just commented out). At some point I'd like to port purescript-lists and purescript-transformers (but purescript-transformers might be a pain).

@cdepillabout cdepillabout mentioned this issue Jan 23, 2022
48 tasks
@m-bock
Copy link

m-bock commented Jan 23, 2022

@cdepillabout great, thanks for the invite. I read the MEMBERS guide and agree to it. Invite is already accepted.

Regarding the spago-test.dhall, it's unrelated to #39 . It's just an attempt to separate library dependencies from test dependencies. See e.g. this section from the spago manual. In this document it's suggested to split into a spago.dhall and a test.dhall. The downside of this approach is that IDE's like vscode with the purescript-ide extension, per default they run spago build --purs-args --json-errors to build the project, which builds spago.dhall. That would mean that the IDE does not work correctly for the test suite.

So the idea was to have a

  • spago-test.dhall: Contains all test dependencies
  • spago-lib.dhall: Contains all library dependencies
  • spago.dhall which is the union of the above two, it's main purpose is to build make the default command in e.g. IDEs build everything.

In the end, it's not so crucial, because the real source of truth of a package's dependencies lives inside the the package set, so those dhall files in the repo are just for the user. And thus, an alternative approach would be to have one single spago.dhall file which is structured like:

let libDeps = [ "prelude" ] -- potentially more...
let testDeps = ["miraculix-lite"] -- potentially more...
in
{ name = "purescript-foreign"
, dependencies = libDeps # testDeps
, backend = "purenix"
, packages = ./packages.dhall
, sources = [ "src/**/*.purs" ]
}

libDeps would have to be in sync with the dependencies listed in the package set.

If you go with just one conventional spago.dhall file (without the let/in like in my example), it would make the manual process of keeping the package set in sync with the deps listed in the repo much harder. It would mean that you'd have to filter out the test dependencies by hand every time.

Thinking about this now, maybe the "let/in" approach is simpler. But I am not sure.

@cdepillabout
Copy link
Member Author

If you go with just one conventional spago.dhall file (without the let/in like in my example), it would make the manual process of keeping the package set in sync with the deps listed in the repo much harder. It would mean that you'd have to filter out the test dependencies by hand every time.

I also realized this when porting some of the PureNix libraries! It is quite annoying to have the spago.dhall file in the repo, and the package definition in the package set be different, since you have to remember to remove test dependencies.

It almost seems weird that there doesn't seem to be a set approach to this in the upstream PureScript packages.

Thinking about this now, maybe the "let/in" approach is simpler.

This is basically what I was considering doing (just to have less files in the repo). Want to try to keep to this style for new libraries we port?

@m-bock
Copy link

m-bock commented Jan 30, 2022

Yes, It think it's simpler with one file. And no real practical benefit we'd get if we go with 3 files. See, the above PR.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
enhancement New feature or request good first issue Good for newcomers help wanted Extra attention is needed
Projects
None yet
Development

No branches or pull requests

3 participants