Clear separation of gollum-lib, adapter specs and adapters

[SkyCrawl]

Hey @dometto ,

i just delved into how things work on the lowest level of Gollum (gollum-lib and the underlying git layer) and I'd like to ask your opinion on the current situation.

To me, it seems (but I may be wrong) overly complicated/confusing and (again :( ) a bit of a mess, especially because of the following:

1. Some bits seem to be defined in gollum-lib and some others in the git layer and both pieces are used in the other...
2. Currently, there are 2 adapters working with gollum (with a third on the way), both of which seem to conform to adapter_specs.

In my opinion:

• Things like Gollum::BlobEntry should be moved from gollum-lib to adapter_specs (gollum-lib should require and use an adapter, just like it is now, but all git-related types and methods should be defined there),
• adapter_specs seems overly complicated and should be rewritten (but I may be wrong on this one),
• I imagine it would be a lot better if adapter_specs defined fully documented communication between underlying git layer and gollum-lib (types and module methods), instead of some "vague descriptions" (if possible, of course). In other words:
  • gollum-lib would only reference methods and instance variables defined in adapter_specs and its types,
  • all methods of adapter_specs should only define arguments and return objects of types predefined in adapter_specs (or primitive types),
  • all methods that require custom implementation should have their skeleton defined (and documented) in adapter_specs.

I think that this way:

• the separation would work so much better,
• tuning and updating the API would be so much easier,
• development of new adapters would be so much easier (if only by having part of the code included from adapter_specs or forking it directly).

While the current solution may not be as bad as I picture it and I may be the one having problems here, has anyone given any consideration to the above in the past?

I don't mean to offend or criticize anyone and especially you, so don't take it personally please, but I'm actually very surprised you (all of you) got Gollum working like this. The way I see Gollum, so much of it adds so much complexity for further development/maintenance and makes the developers (including me, as it would seem) spend so much more time in the process :(...

[dometto]

@SkyCrawl, the adapter specs can be much better and functional, that's agreed. However, I don't find that to be a priority right now, since the functional tests are done in gollum-lib.

On the overall structure: the adapters actually make everything a lot more maintainable than it used to be, when gollum was making direct calls to grit all over the place. This has allowed us to close a lot of bugs (in total we've close over a 100 over the past year, although not all were related to grit and the backend of course). The priority was to end the way gollum was intertwined with grit, without breaking existing functionality and tests, and we've succeeded at that. The next step can be to further abstract and simplify the method signatures and such that still mimic grit to an unnecessary degree. But small steps is the key, and I think a lot of improvements to gollum are possible without first improving the git abstraction layers. As @sunny said in a related thread, changes should be piecemeal and we should keep backwards compatibility as long as possible. I think another big change set addressing the whole of gollum-lib and the adapters would be undesirable.

[SkyCrawl]

Thank you, @dometto .

However, I don't find that to be a priority right now

Neither do I, for that matter :). I should be able to somehow refactor my fork of gollum-lib for the changes I'm making but the design really struck me heavily when I finally looked at it a bit closer...

On the overall structure: the adapters actually make everything a lot more maintainable than it used to be, when gollum was making direct calls to grit all over the place.

Yes, that probably partially explains the current state then. You managed to get rid of most of the mess but didn't get rid of it fully :).

But small steps is the key,

changes should be piecemeal and we should keep backwards compatibility as long as possible.

I think another big change set addressing the whole of gollum-lib and the adapters would be undesirable

In general, I agree... but it seems to me that in Gollum's case, only relatively small improvements can be made while keeping absolute backward compatibility (and not affecting the other gems). I may be a first-hand witness to that :). In my case, one "small" improvement (converting CSS to SASS; perhaps a little surprisingly) triggered a fantastic chain of improvements that are often very much linked together, and many times even required :p.
For evolution in small steps, a well designed and maintainable application is, in my opinion, a must. Unless you have a wish for the evolution to stop rather sooner than later.

Please, don't perceive what I'm about to say as an indication that this issue should be dealt with soon... I have more than enough Gollum-related work on my hands already as things stand now and I'm sure you do as well :). However, while I understand your hesitations in making big changes (truly, I do), I can't help but feel that you (not only you) also greatly underestimate the power of a well written and designed application. I find it's much more efficient (especially for us, developers) to redesign/rewrite an application in a single go and while we're at it, take our time and make sure it is designed and written really well :). Otherwise, the chain of bad design and coding will go on and the longer it goes on, the more painful it will be to:

• upgrade the application to use new technologies,
• restore order and good design,
• keep the user-base.

If things are designed and written well, having to be break backward compatibility again should be limited to very very rare cases (verging on "practically never") if we're careful, not to mention a big bunch of other advantages. Users will be mad that they will have to make changes as well to keep track, that's for sure and we should apologize for that sincerely but it's inevitable in many an application. On the other hand, many of them will also see how much better the new version is and happily welcome many of the changes. Especially so if they allow unprecedented things and will more or less guarantee that users' Wikis won't have to be upgraded again in a few years.

I think users are much smaller a problem than you (not only you) make them to be :). They can always choose to keep the old versions should they wish to do so and they have the freedom to choose to port their Wikis at any time in the future. Making a whole new generation of Gollum would probably shake some users off and most definitely also attract some new users.
Unlike the users, however, for developers, fixing bugs and improving a not well designed application is probably a painful process :). Not to mention individual improvements probably scare them off often because they can't predict, how much the application will change overall. This is a daring thing to say and forgive me for my honesty please, but that may be the main reason why Gollum has so few maintainers and so many forks :).

[dometto]

In my case, one "small" improvement (converting CSS to SASS; perhaps a little surprisingly) triggered a fantastic chain of improvements that are often very much linked together, and many times even required

I know that feel. ;)

Yes, that probably partially explains the current state then. You managed to get rid of most of the mess but didn't get rid of it fully :).

True, although not getting rid of it all at the same time was intended.

I can't help but feel that you (not only you) also greatly underestimate the power of a well written and designed application.

To the contrary -- that was the point of the (rather large) effort to build the adapters/abstraction layers in the first place. To my mind keeping gollum-lib ignorant about the underlying grit layer is one of the more sound features of the current backend. So:

1. The point about the specs is well taken. I propose to open an issue about that in the adapter_specs repo and address it in due course.
2. Abstracting further away from the grit origins where possible (e.g. in method sigs) is a good thing.

But apart from that I think the abstraction layer is fundamentally ok, and is functioning quite well at the moment. I would say: if you have any concrete problems with stuff the adapters can't currently do, let us know, and we can see how best to solve that. :)

Another issue is that it can be annoying for a dev to have to edit/maintain multiple projects/gems. There are a couple of ways the workflow could be improved, e.g., maybe we don't have to ship the default adapter as a separate gem.

[SkyCrawl]

To the contrary -- that was the point of the (rather large) effort to build the adapters/abstraction layers in the first place. To my mind keeping gollum-lib ignorant about the underlying grit layer is one of the more sound features of the current backend.

Indeed but you're probably missing the big scope - the whole chain of gollum, gollum-lib, the specs and, by extension, the adapters as well a bit. I don't claim to be the smartest person there is but to me, much of the design is simply "inadequate" or "wrong" :). That was my original point.

Another issue is that it can be annoying for a dev to have to edit/maintain multiple projects/gems. There are a couple of ways the workflow could be improved, e.g., maybe we don't have to ship the default adapter as a separate gem.

It probably is annoying but in my opinion, it's still better than having the adapter code in gollum-lib. Not to mention that adapter specs should probably live an independent life, and by extension, adapters too :). I like the improvement you made back then... it was really a step towards a shiny future 👍 .

[dometto]

Closing in favour of gollum/adapter_specs#3