refactor: Extract Defra specific logic from ACPLocal type

[AndrewSisley]

Relevant issue(s)

Resolves #2655

Description

Extracts Defra specific logic from the ACPLocal type so that it may be re-used for SourceHub ACP.

None of the code has changed, it's just moved around.

I'm opening a PR now, as a lot of us (including Bruno) are working in this space, and merging refactors like this asap tends to minimise conflicts.

The (WIP) source hub implementation can be found here: #2657

[shahzadlone]

I wonder if we are over abstracting things by having both the ACP and sourcehubClient interfaces. This introduces a complexity which I am not sure we need.

From what I understand so far (correct me if missing something):

• The ACP interface was combining the "wrapping of sourcehub" + "defra specific acp logic"

• But rather than re-implementing the wrapping of sourcehub logic for remote acp and the defra specific acp logic you thought perhaps you can separate the sourcehub client to re-use those parts? Hence the current bridge and new interface?

• The sourcehubClient just seems like a lean wrapper extracting the functions we need from sourcehub. Couldn't this just be a struct? What other thing would implement it other than the sourceHubBridge

[AndrewSisley]

The ACP interface was combining the "wrapping of sourcehub" + "defra specific acp logic"

Not just the ACP interface, the localACP implementation of that interface was. The ACP interface should/can/continues-to do that.

But rather than re-implementing the wrapping of sourcehub logic for remote acp and the defra specific acp logic you thought perhaps you can separate the sourcehub client to re-use those parts? Hence the current bridge and new interface?

If by those parts, you mean the defra specific logic, yes. It is shared 'business logic' that should be the same regardless of what the underlying sourcehub/acp tech is. Later it can be extracted further so that the wrapper does not only function for sourcehub lib based clients, but any acp client - however I don't want to code for the imaginary and over engineer something that wont get used as such for a while.

The sourcehubClient just seems like a lean wrapper extracting the functions we need from sourcehub. Couldn't this just be a struct? What other thing would implement it other than the sourceHubBridge

I think you are confused. sourceHubBridge implements acp.ACP. sourcehubClient is implemented by acpLocal and acpSourceHub.

[shahzadlone]

Thanks for the clarification, so to understand the full picture:

• sourcehubClient will be implemented by acpLocal and acpRemote.
• ACP interface will continue to be the "sourcehub wrapper / bridge (local or global) + defra specfic acp logic"

Is that correct?

[AndrewSisley]

Is that correct?

Yes. We shouldn't expose the internals (client vs defra logic spit) outside of the acp package. You can see the source hub implementation of the client in the PR linked in this PR's description

[shahzadlone]

Thanks for the explanation here and on Discord.

I think ACP interface should be a struct rather than an interface with the new changes, I don't like abstractions at this stage when we only have sourcehub acp that we need to worry about now, mostly for the sake of simplicity.

But the preference isn't a hard blocker however. So will give an approval, would be nice to hear from others too.

[AndrewSisley]

I think ACP interface should be a struct rather than an interface with the new changes,

I do strongly disagree with that. acp.ACP is a public type and users should be free to provide their own. The only implementation we have is very much an internal construct too, and we have plans to add more implementations. We also really don't want to couple every referencing location to a concrete type - it makes changing the acp implementation much more expensive.

[AndrewSisley]

would be nice to hear from others too.

I'm very happy to continue this discussion post-merge, changing that after this goes in will probably cause far less problems than delaying this merge. I would like to merge this before I start my weekend too (I don't want to have this PR in my head next week).

[jsimnz]

Want to briefly chime in and say I agree with @AndrewSisley regarding the interface vs struct approach.

I'm not a huge fan of the internals of some of this PR regarding the client approach, but it's fine and wouldn't have caused me to block the merge anyway.