Skip to content

feat: multiple editable modules#429

Merged
mergify[bot] merged 6 commits intomainfrom
brprice/multiple-home-modules
May 11, 2022
Merged

feat: multiple editable modules#429
mergify[bot] merged 6 commits intomainfrom
brprice/multiple-home-modules

Conversation

@brprice
Copy link
Copy Markdown
Contributor

@brprice brprice commented May 5, 2022

Support multiple editable modules. Previously one could import as many modules as wanted, but only had one module to write code in. This is now relaxed so writing larger, multi-module programs is possible without abusing imports.

Closes PRIM-23 and PRIM-76

@brprice brprice force-pushed the brprice/multiple-home-modules branch from 3cab5e9 to 93a7d48 Compare May 6, 2022 13:12
@brprice brprice changed the base branch from main to brprice/update-api-prog-comment May 6, 2022 13:13
@brprice
Copy link
Copy Markdown
Contributor Author

brprice commented May 6, 2022

@dhess I'm seeing a unrelated (I think) rel8 test suite failure: https://buildkite.com/hackworthltd/primer/builds/603#67bb9eff-aadb-4da6-8710-a0476d028ce0, which says

    ListSessions
      listSessions:                              FAIL (4.74s)
        Insert all sessions   (2.38s)
        Get all, offset+limit
        Get 25
        Get 76-100
        Get crossing end
          /tmp/tmp-postgres-data-a61157f8685c7193/pg_stat: removeDirectoryRecursive:removeContentsRecursive:removePathRecursive:removeContentsRecursive:removeDirectory: unsatisfied constraints (Directory not empty)
        
        Use -p '/listSessions/' to rerun this test only.

I'll restart the test to see if it is reproducible.
Edit: Retrying the test has passed.

@dhess
Copy link
Copy Markdown
Member

dhess commented May 6, 2022

Hmm, that used to occur from time to time due to a race condition in tmp-postgres, but I thought I'd worked around it. Let's keep an eye on it.

@brprice brprice force-pushed the brprice/multiple-home-modules branch 2 times, most recently from c52eb00 to 74f738f Compare May 6, 2022 16:10
Base automatically changed from brprice/update-api-prog-comment to main May 9, 2022 12:29
@brprice brprice force-pushed the brprice/multiple-home-modules branch from 74f738f to e4a2bc7 Compare May 10, 2022 10:37
@brprice brprice changed the base branch from main to brprice/AddConField-fixes May 10, 2022 10:37
@brprice brprice marked this pull request as ready for review May 10, 2022 10:50
@brprice
Copy link
Copy Markdown
Contributor Author

brprice commented May 10, 2022

This is ready for review. This implements the FR #321

@brprice brprice linked an issue May 10, 2022 that may be closed by this pull request
@brprice brprice requested a review from a team May 10, 2022 10:53
@brprice
Copy link
Copy Markdown
Contributor Author

brprice commented May 10, 2022

There is something a bit odd in CI: https://buildkite.com/hackworthltd/primer/builds/650#e19733c0-f807-45f2-b4ba-26b67caaf831 "Unexpected lambda in case", and then some GHC Core is output (but CI still passed). I'll investigate, but I don't think this should delay review.

Since CI still passed, I'm not sure whether this is a new symptom. It was just luck that I happened to look at CI to check progress at the right time to notice it. Edit: Looks like it only occured on the last commit (3d54b9f)

@dhess
Copy link
Copy Markdown
Member

dhess commented May 10, 2022

I wonder if it's due to a dependency bump via 69cce80

edit: Per discussion offline with @brprice, sounds like it's not, as he hadn't rebased this PR on main when the CI issue occured.

@brprice brprice force-pushed the brprice/AddConField-fixes branch from e42b7a0 to 4511478 Compare May 10, 2022 12:41
@brprice brprice force-pushed the brprice/multiple-home-modules branch from 3d54b9f to 9cdcda3 Compare May 10, 2022 12:45
We consistently handle "not found" errors. This is in preparation for
adding multiple editable modules. Thus we add some new errors
`ModuleNotFound` and `ModuleReadonly` and use these as appropriate. Note
that this may change some errors the API returns.
@brprice brprice force-pushed the brprice/multiple-home-modules branch from 2516fb8 to b4e0d3b Compare May 10, 2022 12:50
brprice added 5 commits May 10, 2022 13:50
Currently they will return an error if they target anything other than
the one editable module. However, we will shortly enable multiple
editable modules, at which point we need to explicitly state what module
they should affect.

BREAKING CHANGE: this change requires a database migration, as it
changes the representation of `Log`. However, since this is just
serialised to json and stored as a blob in the DB, it requires no schema
changes. Since we have no programs we need to preserve, we decided not
to bother with a migration. This means that DBs created before this
commit will not load with a primer containing this commit. This commit
also changes Primer's API.
Previously we only viewed the contents of the one editable module.
We now give the contents of every module, and also tag them with whether
they are editable or imported.

BREAKING CHANGE: this changes the response to an api endpoint, and this
is reflected in the generated openapi schema.
NB: lots of this commit is fairly trivial testsuite churn

BREAKING CHANGE: this change requires a database migration, as it
changes the representation of `Prog`. However, since this is just
serialised to json and stored as a blob in the DB, it requires no schema
changes. Since we have no programs we need to preserve, we decided not
to bother with a migration. This means that DBs created before this
commit will not load with a primer containing this commit.
@brprice
Copy link
Copy Markdown
Contributor Author

brprice commented May 10, 2022

I have rebased onto 69cce80 and it looks like the GHC message has disappeared:

I wonder what change caused this (edit: from some investigation, it seems that multiple GHCs will emit this message in certain circumstances, and it is rather sensitive to small code changes.)

@linear
Copy link
Copy Markdown

linear Bot commented May 10, 2022

PRIM-23 Modules phase 2

  • Naming modules
  • Breaking programs into modules

PRIM-76 Multiple editable modules

@dhess
Copy link
Copy Markdown
Member

dhess commented May 10, 2022

For anyone who reviews this after me (including @georgefst), this is one PR where I found it very helpful to turn on GitHub's "Hide whitespace" option, as there are otherwise large chunks of code that are unchanged save for indentation.

Comment thread primer/src/Primer/App.hs
Comment thread primer/src/Primer/Action.hs
@dhess
Copy link
Copy Markdown
Member

dhess commented May 10, 2022

I'm afraid I'm too far removed now from Primer core to have anything particularly interesting to say, but this all looks fine to me. Let's obviously wait for @georgefst to weigh in.

Copy link
Copy Markdown
Contributor

@georgefst georgefst 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!

Base automatically changed from brprice/AddConField-fixes to main May 11, 2022 15:55
@brprice brprice added the Ready to merge Ready to merge label May 11, 2022
mergify Bot added a commit that referenced this pull request May 11, 2022
@mergify mergify Bot merged commit 7ccb043 into main May 11, 2022
@mergify mergify Bot deleted the brprice/multiple-home-modules branch May 11, 2022 16:03
@brprice
Copy link
Copy Markdown
Contributor Author

brprice commented May 11, 2022

I have rebased onto 69cce80 and it looks like the GHC message has disappeared:

* https://buildkite.com/hackworthltd/primer/builds/665#c977a497-c194-4975-a385-d947b8fbccdd

* https://buildkite.com/hackworthltd/primer/builds/665#9c764e17-c92f-422f-946f-adaffacdf132

I wonder what change caused this (edit: from some investigation, it seems that multiple GHCs will emit this message in certain circumstances, and it is rather sensitive to small code changes.)

I think there is nothing wrong with our code, and am filing it as a ghc issue: https://gitlab.haskell.org/ghc/ghc/-/issues/21555

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Labels

Ready to merge Ready to merge

Projects

None yet

Development

Successfully merging this pull request may close these issues.

Multiple editable modules

3 participants