Skip to content

Claudio/collection modules#220

Merged
matthewhammer merged 54 commits intomasterfrom
claudio/collection-modules
Mar 11, 2019
Merged

Claudio/collection modules#220
matthewhammer merged 54 commits intomasterfrom
claudio/collection-modules

Conversation

@crusso
Copy link
Contributor

@crusso crusso commented Mar 7, 2019

Matt, I've refactored the libraries to just used an immediate module construction. It gets rid of the boilerplate at the end if each library and avoid the Lib__ naming. See if you like it.

@crusso crusso requested a review from matthewhammer March 7, 2019 12:38
@crusso crusso mentioned this pull request Mar 7, 2019
@nomeata
Copy link
Contributor

nomeata commented Mar 7, 2019

Did you want this to be a PR against Matt’s branch? In that case, use the “Edit” button on top and change the target branch.

Copy link
Contributor

@matthewhammer matthewhammer left a comment

Choose a reason for hiding this comment

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

These changes are great! The resulting code is much simpler and cleaner.
Thanks, @crusso!

@matthewhammer
Copy link
Contributor

One minor question: Are we trying to merge into master, or into collection-modules, here?

@matthewhammer
Copy link
Contributor

Hmm...perhaps it's time to start having the seeds of a quasi-official collections library in master?

Then, I can start working on the produce exchange example by doing PRs against master as well.

And, since all of this standard library work (current and future) is mostly independent of making changes to the compiler source, there's little harm in merging into master, rather than my working branch; I'd be happy to abandon these side branches, in preference of smaller PRs against master in the future.

What do other people think?

cc @nomeata @ggreif @rossberg

@crusso crusso changed the base branch from master to collection-modules March 7, 2019 14:04
@crusso
Copy link
Contributor Author

crusso commented Mar 7, 2019

Oh, my intention was to merge into matt's collection-modules. It was just meant as a proposal. I'll retarget and @matthewhammer can decide what to with it.

@matthewhammer, I'm fine with you merging into master. Maybe the libraries should live under a meaningful folder though, like lib (perhaps do whatever the ocaml repo does).

@nomeata
Copy link
Contributor

nomeata commented Mar 7, 2019

I am fine having it in master, if all code is actually built by our CI infrastructure, so that it does not rot.

@matthewhammer matthewhammer changed the base branch from collection-modules to master March 7, 2019 14:53
@matthewhammer
Copy link
Contributor

OK, I've moved things, per the slack discussion:

  • stdlib is a new sibling of samples and test, etc
  • stdlib/examples contains one (still nascent) example, the produce exchange.

Arguably, the examples subdirectory should really be merged somehow with samples subdirectory, except the latter is meant (I think) to contain self-contained samples, whereas the former are meant to all depend on the new standard library in very non-trivial ways, as will be the case with the produce exchange.

What do people think? Can we merge into master now? I'm happy to revisit this organization question later.

Copy link
Contributor

@nomeata nomeata left a comment

Choose a reason for hiding this comment

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

Please make sure it is built from default.nix, in derivation native_test, just like test/ and samples/.

stdlib/Makefile Outdated
@@ -0,0 +1,110 @@
ASC=../src/asc
OUTDIR=out
Copy link
Contributor

Choose a reason for hiding this comment

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

In the other directories, this is _out (not my convention, but let’s stick to it).

You should add a .gitignore here that ignores that directory.

Copy link
Contributor

Choose a reason for hiding this comment

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

Ok, sure.

@@ -0,0 +1,109 @@
/*
Copy link
Contributor

Choose a reason for hiding this comment

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

What is the meaning of a nonCriticalPath subdirectory?

Copy link
Contributor

Choose a reason for hiding this comment

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

A temporary receptacle for code that I wrote, but that isn't "on the critical path" of what I need to implement the produce exchange.

I'm happy to remove it entirely, if that's preferable. Or, rename this subdirectory to something more suggestive of its role.

@matthewhammer matthewhammer requested a review from nomeata March 7, 2019 19:34
Copy link
Contributor

@nomeata nomeata left a comment

Choose a reason for hiding this comment

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

LGTM, thanks!

Copy link
Contributor

@ggreif ggreif left a comment

Choose a reason for hiding this comment

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

I'd love this to be tested in CI! Please merge soon.

@matthewhammer matthewhammer merged commit f31baf1 into master Mar 11, 2019
@nomeata nomeata deleted the claudio/collection-modules branch April 18, 2019 13:04
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.

4 participants