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

refactor: carve out modules from Main.hs #1814

Merged
merged 4 commits into from
Apr 24, 2021

Conversation

monacoremo
Copy link
Member

@monacoremo monacoremo commented Apr 13, 2021

This will reduce the amount of code under conditional CCP compilation, making the code easier to reason about. Carving out modules will also increase unit testing opportunities.

Very WIP.

Edit: I converged to the following structure, to summarize:

  • PostgREST.AppState encapsulate all stateful pieces of our application. It's now the only module handling MVars, IORefs etc. This makes it very obvious where state is used in other modules, and makes it easier to refactor the state further.
  • PostgREST.Workers contains all current workers. They seem a bit convoluted right now, I tried to clarify the current setup by avoiding passing around connWorker :: IO () etc. More work to be done here to simplify
  • PostgREST.Config has a simplified interface - parsing the config is now one function, not puzzling together with the right incantation of multiple functions. Config could be signifcantly refactored now that most (soon: all?) options are optional and we have several config sources (env, config file, db..)
  • PostgREST.Config.Database contains a few of the config from db pieces (e.g. the statement, that was a bit out of place with the request/query stuff) - more to come as we refactor Config down the line
  • PostgREST.Unix now contains all the unix-specific functionality (from UnixSocket and things in old Main)
  • PostgREST.CLI absorbed a bit more of the miscellaneous functionality of the executable. To be further sorted out as we go
  • Main now is quite minimal, also regarding external dependencies. Minimal code is under conditional CPP compilation

main/Main.hs Outdated Show resolved Hide resolved
@monacoremo
Copy link
Member Author

I've added a summary of what changed to the top comment. Suggest that we make a cut here, WDYT @wolfgangwalther ? Thank you for all the review!!

@monacoremo monacoremo marked this pull request as ready for review April 19, 2021 19:40
Copy link
Member

@wolfgangwalther wolfgangwalther left a comment

Choose a reason for hiding this comment

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

I did it! My head is burning a bit, because this a bit harder to review, but I'm still alive.

It's a bit annoying that this is one of the areas where we're really missing a few tests. Need to review a bit more carefully to make sure there is no change in behaviour. Refactoring with 100% coverage is a lot more fun ;)

main/Main.hs Outdated Show resolved Hide resolved
main/Main.hs Show resolved Hide resolved
main/Main.hs Outdated Show resolved Hide resolved
main/Main.hs Outdated Show resolved Hide resolved
src/PostgREST/CLI.hs Outdated Show resolved Hide resolved
src/PostgREST/Workers.hs Outdated Show resolved Hide resolved
src/PostgREST/Workers.hs Outdated Show resolved Hide resolved
src/PostgREST/Workers.hs Outdated Show resolved Hide resolved
src/PostgREST/Config.hs Outdated Show resolved Hide resolved
src/PostgREST/Config.hs Outdated Show resolved Hide resolved
@wolfgangwalther
Copy link
Member

Suggest that we make a cut here, WDYT @wolfgangwalther ?

Yes, please! :D

@monacoremo
Copy link
Member Author

Suggest that we make a cut here, WDYT @wolfgangwalther ?

Yes, please! :D

Thank you so much for your thorough review! Working on the fixes

@monacoremo
Copy link
Member Author

@wolfgangwalther I think that's it! All review comments should be adressed, in individual commits since you reviewed. I didn't touch the history. Shall we merge?

@wolfgangwalther
Copy link
Member

@wolfgangwalther I think that's it! All review comments should be adressed, in individual commits since you reviewed. I didn't touch the history. Shall we merge?

Apart from the still wrong comment, everything else looks good.

Could you rebase/squash in a way, that everything is in one refactor: commit, except:

  • fix: Improve log message on config reload
  • fix: Close unix socket properly
  • fix: Panic when running on windows with unix socket enabled

(I hope I got all of those?)

Would be good to separate those in the history, because they change behaviour. And we can then put them in the changelog, too, when they are properly prefixed and separated.

@monacoremo monacoremo force-pushed the slimmain branch 2 times, most recently from 765b7dc to e861a16 Compare April 24, 2021 16:43
@monacoremo
Copy link
Member Author

Would be good to separate those in the history, because they change behaviour.

Yes, that's good! Everything else was just reshuffling stuff around.

@wolfgangwalther, I hope you will not be too sad, I think this was the last mega-refactor for now. Now that we have split up a lot of stuff, the further changes will be more localized. Thank you so much for bearing with me on this!

@wolfgangwalther
Copy link
Member

Something went wrong with rebasing. The always pass is back on the windows branch.

@monacoremo
Copy link
Member Author

Something went wrong with rebasing. The always pass is back on the windows branch.

Yes, weird - must have slipped by as I reordered the commits, would have expected a rebase conflict. Added it back in.

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

Successfully merging this pull request may close these issues.

3 participants