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

Clean up Wire.Subsystem module structure #4081

Draft
wants to merge 2 commits into
base: develop
Choose a base branch
from

Conversation

fisx
Copy link
Contributor

@fisx fisx commented Jun 4, 2024

https://wearezeta.atlassian.net/browse/WPB-9597

(inspired by https://wearezeta.atlassian.net/browse/WPB-8880)

Checklist

  • Add a new entry in an appropriate subdirectory of changelog.d
  • Read and follow the PR guidelines

@zebot zebot added the ok-to-test Approved for running tests in CI, overrides not-ok-to-test if both labels exist label Jun 4, 2024
@fisx fisx force-pushed the fisx/refactor-subsystems branch 3 times, most recently from 0d0267e to ca91152 Compare June 6, 2024 10:36
@fisx fisx force-pushed the fisx/refactor-subsystems branch from ca91152 to 5738080 Compare June 11, 2024 13:13
@MangoIV
Copy link
Contributor

MangoIV commented Jun 11, 2024

I have a couple of questions:

  • are we going to update the output of tree every time we're modifying the subsystem? how are we verifying that?
  • why is it called Eff? I think it's misleading, many effect systems call their monad Eff.
  • why is it called Mem? Wouldn't InMemory convey the semantics better?

I somehow like the idea of a flatter directory structure so I think that UserSubystem is nicer that Subsystem.User but also it feels like it doesn't adhere to the conventions; perhaps we could even do WireSubsystem if we don't adhere to them anyways?

@akshaymankar
Copy link
Member

Could you please write about the motivation for this work, nothing in JIRA or README or the PR description seem to have it captured, so its hard to even engage with this PR. Still here are somethings that could make it better:

  1. I think instead of the tree output we could put slightly more effort into just listing the module namespaces which matter most and write about them, the tree output is just noisy for most part.
  2. Calling things Subsystem.User instead of UserSubsystem makes a lot of files in the code being called User.hs and (fuzzily) searching for them is a pain (also most editors just show file names when there are multiple buffers or tabs are open), so I also prefer UserSubsystem from dev-ex perspective.
  3. I also don't have very strong conviction about which things are Subsystems and which things are not (specially the not part). If we cement things down with this directory structure now, it feels a bit premature, I think we should work on defining those first (or perhaps spend some more time with this to get the sense).

All said, it would be best to first define our reasons for this then spend more time into it if we feel its justified.

@fisx
Copy link
Contributor Author

fisx commented Jun 12, 2024

  • are we going to update the output of tree every time we're modifying the subsystem? how are we verifying that?

no, we'll either keep it as an incomplete sample, marking it as such, or remove it entirely. it's not that hard to look for yourself after all. i just put it there for now so people can see what i mean.

  • why is it called Eff? I think it's misleading, many effect systems call their monad Eff.

no strong opinion / open to suggestions. the reason was that Eff interpreters only implement one polysemy effects in terms of others, so it's not cassandra, but a polysemy Effect underneath.

what about Polysemy?

  • why is it called Mem? Wouldn't InMemory convey the semantics better?

sure, InMemory is at least as good. i'll take it, thanks! :)

@echoes-hq echoes-hq bot added echoes: throughput Changes intended at preserving our ability to evolve the software safely and effectively echoes: technical-debt Changes intended at mitigating risks labels Jun 27, 2024
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
echoes: technical-debt Changes intended at mitigating risks echoes: throughput Changes intended at preserving our ability to evolve the software safely and effectively ok-to-test Approved for running tests in CI, overrides not-ok-to-test if both labels exist
Projects
None yet
Development

Successfully merging this pull request may close these issues.

4 participants