Spike refactor the app #798
Conversation
There was a problem hiding this comment.
In general this looks good to me, but it would be worth running it by someone with more detailed knowledge of the app, e.g. @h-lame, before merging.
| @@ -0,0 +1,9 @@ | |||
| class PublishingApi | |||
| def self.instance | |||
| GdsApi::PublishingApi.new( | |||
There was a problem hiding this comment.
I think it would be better to be consistent about memo-izing the singleton instance as you did for RummagerApi in this commit, i.e. as per the implementation of SingletonDependency#get.
| @@ -1,3 +1,5 @@ | |||
| require "gds_api/publishing_api" | |||
|
|
|||
There was a problem hiding this comment.
I assume you intend to squash this into the previous commit...?
|
|
||
| class PublishingApiV2 | ||
| def self.instance | ||
| GdsApi::PublishingApiV2.new( |
There was a problem hiding this comment.
I'd be 100% surprised if we need publishing_api and publishing_api_v2. I suspect we can drop the former, and rename the latter to publishing_api
There was a problem hiding this comment.
Same comment as before.
Addressed in 8592ac5.
There was a problem hiding this comment.
I'd be 100% surprised if we need
publishing_apiandpublishing_api_v2. I suspect we can drop the former, and rename the latter topublishing_api
Addressed by opening #809.
|
|
||
| class OrganisationsApi | ||
| def self.instance | ||
| GdsApi::Organisations.new(ORGANISATIONS_API_BASE_PATH) |
| def self.instance | ||
| @rummager_api ||= GdsApi::Rummager.new(Plek.new.find("search")) | ||
| end | ||
| end |
There was a problem hiding this comment.
A pattern that other gov.uk apps have been using is a Services module that has class methods on it for each gds-api used by the app. Might be worth pushing this as far as that too. For example: https://github.com/alphagov/policy-publisher/blob/0f2fa349a043ae7acea37e22bbb08b57154fb30e/lib/services.rb
There was a problem hiding this comment.
Addressed by opening #811.
| MarkdownAttachmentProcessor.method(:new), | ||
| SpecialistDocumentHeaderExtractor.create, | ||
| GovspeakToHTMLRenderer.create, | ||
| ] |
There was a problem hiding this comment.
It feels like it might be a bug that this and the ManualDocumentRenderer you deal with in the next few commits are subtly different; this one doesn't do FootnoteSomethingSomething in its pipeline, but the ManualDocumentRenderer does. We should check obviously, but I think we can get rid of this one and replace it's single use with an instance of the ManualDocumentRenderer instead.
There was a problem hiding this comment.
Addresses by opening #810.
|
I think what you've done so far looks good. My comments are mostly about where you could push it further next if you plan to keep iterating on this spike. If you think you're done for now though I don't think it would hurt to merge (maybe wait until #799 is merged and deployed though). |
|
Thanks for looking at this @floehopper and @h-lame. Do you think it's worth the effort of tidying this branch up and merging it? Does the approach in general leave the code in an improved-enough state to warrant more work? |
|
Note that some of the early commits in this branch (those that remove code) have been moved to PR #801. |
The `:manual_builder` and `:manual_document_builder` are the only entries in `ManualsPublisherWiring.get` whose name ends in `_builder`. Making this explicit should help me remove the `ManualsPublisherWiring` in future.
TODO: Consider splitting this so that the first commit adds `ManualBuilder.create` and the next inlines `:manual_builder`.
Note that `ManualPublishingApiBulkDraftExporter` takes `wiring` as a parameter. I'm presuming that it's an instance of `ManualsPublisherWiring`. I'm not worried about investigating too much further as `ManualPublishingApiBulkDraftExporter` doesn't appear to be instantiated anywhere anyway!
This does not appear to be used.
As in the commit title "Inline :repository_registry", I'm assuming that the `wiring` passed to `ManualPublishingAPIBulkDraftExporter` is a `ManualsPublishingWiring`. I can't be sure because `ManualPublishingAPIBulkDraftExporter` doesn't appear to be instantiated.
NOTE. This is a singleton factory.
Now that it's no longer being used.
chrisroos
force-pushed
the
spike-refactor-the-app
branch
from
193f59e
to
3be562e
Compare
|
I've rebased this on master and force pushed now that a number of the other PRs have been merged. |
|
I've rebased this branch on master, addressed some of the concerns raised in the comments and opened PR #808 which supersedes this PR. |
I'm using this spike to explore whether it's worth trying to simplify the app.
In this branch I:
ManualsPublisherWiringManualsPublisherWiringandDependencyContainerI'm pushing up this work in progress branch to get some feedback about the approach.