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

Collapse relation, mapper and session into a single repo #12

Closed
solnic opened this issue Jan 24, 2014 · 17 comments
Closed

Collapse relation, mapper and session into a single repo #12

solnic opened this issue Jan 24, 2014 · 17 comments

Comments

@solnic
Copy link
Member

solnic commented Jan 24, 2014

Refactoring in rom-mapper made me realize it's really a good idea to collapse all 3 pieces into a single repo as there are still way too many moving parts and I find it a PITA to manage 4 projects separately. It would also give better visibility of what you are breaking when introducing refactorings like in this branch (it breaks integration with both rom-relation and rom-session). I also don't think anybody will use session or mapper or relation as standalone gems, honestly. ROM is clearly becoming a convenient DSL on top of lower-level tools (axiom, morpher, probably more will come) so I don't see any point in maintaining complexity that comes with the separation. It is also way easier for people to understand the project when it's in a single repo. We can always extract things later when we see that it makes sense (I don't see any sense at the moment).

@tjstankus
Copy link

👍

@gerhard
Copy link

gerhard commented Jan 24, 2014

If it's a PITA, it's wrong. Good call, can definitely relate to it.

@solnic
Copy link
Member Author

solnic commented Jan 24, 2014

Yeah, theoretically this separation is logical and correct but in practice it's been a pain to deal with those 3 repos from my point of view. Hence I'm starting the discussion.

Thanks for feedback!

@mbj
Copy link
Contributor

mbj commented Jan 24, 2014

@solnic I'm agree. As long as we continue to factor out "stable" parts into separate gems/repos (like we did with adamantium, equalizer, anima) (that all had been part of a rom related repo once).

@solnic
Copy link
Member Author

solnic commented Jan 24, 2014

@mbj yeah I like that you used the word "stable". it's a key word in this context - ROM is extremely unstable (in terms of API) so keeping things separate so prematurely is a big no-no for me and that's an opinion based on practical experience while working on those projects.

@mbj
Copy link
Contributor

mbj commented Jan 24, 2014

@solnic You agree stuff like sql, equalizer, adamantium, anima, ice_nine is exactly of the size and "stable"-state it deserves an own repo? - Than we are on the same page.

sql is special, its under rapid development, but this gem has, by definition, a really narrow interface. So it must not be stable to fulfill the "own repo" requirements.

@solnic
Copy link
Member Author

solnic commented Jan 24, 2014

@mbj those are great examples of nice libs that were extracted from a living organism (axiom etc)

Here the story is completely different. We don't know what's ROM. We will define it when we make it work according to our initial expectations (using axiom as the backend, mapping tuples into objects, session with UoW). When that happens it will become clear what we can extract.

There are cases that are very obvious (rarely) where you just know something should be a separate library - SQL is a good example. We need something that generates SQL and holds an AST representation of SQL. Simple, we need a library that will do just that. Re-usability of such library is also obvious.

@avdi
Copy link

avdi commented Jan 24, 2014

Since you can always generate multiple gems from a single repo, I don't see a downside...

@solnic
Copy link
Member Author

solnic commented Jan 24, 2014

@avdi that too :)

@snusnu
Copy link
Member

snusnu commented Jan 24, 2014

Yeah, fine with me, a few things tho:

We should have an extraction in mind and not introduce dependencies were they shouldn't be (I'm talking about "inter component" dependencies here). I'm assuming we'd move the relevant code as it is below rom/lib/{mapper, relation, session}? We should keep it so the separate components are requirable on their own and pass their own "isolated" spec suites (subfolders). I'm already using parts of the stack in an app I'm developing, and I don't need rom-session so currently I don't require it. I can imagine various scenarios where rom-session won't be needed. In fact, I can imagine various scenarios where rom-relation won't be needed but only rom-mapper will be, which leads me to my next point.

It's funny, now that you mention combining all the repos into one for now, I have the feeling that we should maybe even (prepare to) split one more piece out of the stack: The DSL on top of morpher that makes it easy to map nested structures using #attribute, #wrap and #group, so basically the DSL currently present in rom-mapper/ducktrap. Such a thing could then be used by rom-mapper to integrate the mapping process with the rest of the stack.

I can imagine various usecases for such a gem outside of the rom stack, in fact, my app could use such a piece already. When my app receives an HTTP request, there's a dedicated step that takes the various params coming in with that request (path, query and body params), deserializes them if necessary, and maps them to a Param object suitable for the specific endpoint that was invoked. Obviously no relations or sessions are involved. It's purely about rejecting structurally invalid input and mapping to a PORO encapsulating all passed in params. Fwiw, validation comes in a later step, so when i talk about structurally invalid, I'm talking about parameter sanitization, the domain morpher was originally designed for, and shines.

I guess what I'm getting at, is that the current rom-mapper/ducktrap code could be the initial code for a gem that provides a morpher DSL targeted for mapping usecases.

@dkubb
Copy link
Contributor

dkubb commented Jan 24, 2014

I'm fine with this as long as we keep following the conventions we've established so far:

  1. Things not strictly related to the gem are put in support/ and are decoupled from the rest of the code. They should be allowed to mature to the point where they can be extracted, as time allows.
  2. Make sure dependencies between the primary components are minimized as much as possible and try to isolate the spec suites (as @snusnu recommends).

For the second point, maybe we should have unit tests require just the class under test (and in limited cases required dependencies), and no longer rely on requiring the entire lib all at once in spec_helper.rb (?). This would allow us to run each unit test in isolation if we wish and naturally minimize dependencies.. and when there are dependencies, they'll be painfully obvious because the specs will have tons of require statements at the top pointing out the tight coupling.

@solnic
Copy link
Member Author

solnic commented Jan 24, 2014

My intention is not to couple everything together, I just want to deal with one Gemfile and one gemspec and one gem push command. I want to keep things logically split in the repo. It's just way easier to work on stuff when you have it in one place. All those things integrate with each other that's why it's gonna simplify the development.

I like the idea of explicit requires in specs. We can try doing that.

@ghost
Copy link

ghost commented Jan 24, 2014

Throwing this in here half off topic: http://espy.github.io/ubersicht

A dashboard for recent activity on all of a user's or organisation's repos

Regarding the topic, I'm +1 with everything said.

@snusnu
Copy link
Member

snusnu commented Jan 24, 2014

Nice, that's almost like something we've been talking about for ages: http://espy.github.io/ubersicht/#rom-rb

@snusnu
Copy link
Member

snusnu commented Jan 24, 2014

It'd be awesome if it were possible to add repos that do not belong to the same organization tho. That's what we've been talking about all the time at least

@solnic
Copy link
Member Author

solnic commented Jan 24, 2014

@lgierth we centralized issues in this repo some time ago already so it's not...an issue ;)

@solnic
Copy link
Member Author

solnic commented Jan 24, 2014

It's done. Closing. Thanks for the input everyone ❤️

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

No branches or pull requests

7 participants