This repository has been archived by the owner on Mar 6, 2020. It is now read-only.
Detach porcelain from netx/internal #146
Merged
Conversation
This file contains bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
This is an internal implementation detail currently used by x/porcelain. We are planning on forking x/porcelain in probe-engine. So we cannot use any internal package inside its implementation.
We were previously wrapping the error in porcelain, which was not 100% correct, because technically porcelain is just a set of helpers on the top the underlying functionality. Instead, perform this kind of wrapping in the internal RoundTripper. With also this change committed to master, we have severed the internal ties between x/porcelain and the rest of netx. This is functional to my plan of forking x/porcelain inside of probe-engine.
It was not always made obvious enough.
It makes sense to discuss the scoreboard, because this is a functionality that we hope will eventually be stable. All the rest is just experiments and currently both x/porcelain and x/logger are clearly not only track of becoming stable. Hence, do not spend time mentioning them.
This was referenced Jan 8, 2020
Sign up for free
to subscribe to this conversation on GitHub.
Already have an account?
Sign in.
Add this suggestion to a batch that can be applied as a single commit.
This suggestion is invalid because no changes were made to the code.
Suggestions cannot be applied while the pull request is closed.
Suggestions cannot be applied while viewing a subset of changes.
Only one suggestion per line can be applied in a batch.
Add this suggestion to a batch that can be applied as a single commit.
Applying suggestions on deleted lines is not supported.
You must change the existing code in this line in order to create a valid suggestion.
Outdated suggestions cannot be applied.
This suggestion has been applied or marked resolved.
Suggestions cannot be applied from pending reviews.
Suggestions cannot be applied on multi-line comments.
Suggestions cannot be applied while the pull request is queued to merge.
Suggestion cannot be applied right now. Please check back later.
This is yak shaving as part of ooni/probe-engine#2. Specifically, this diff refactors the code such that
netx/x/{logger,porcelain}does not depend onnetx/internal.Each individual commit contains its own short rationale.
On a broader level, this diff is related to discussions I had with @FedericoCeratto concerning unifying
netxandprobe-engineto reduce complexity. I have determined that this won't happen because:I am using
netxalso injafarand it makes sense therefore to keepnetxindependentthe
netxAPI described inDESIGN.mdis not poised to change because it is based on the standard library API, hence we should keep it stableThus, I can see how occasionally we'll make the data model richer, or apply fixes, but I see low coupling between the
DESIGN.mdAPI andprobe-engine.OTOH, the coupling is very high for two packages in
x:porcelainandlogger. So, the right approach is to fork these two packages inprobe-engineand develop them in there. This PR is precisely aiming at making sure that we can fork them. I also considered deleting them but, since they are clearly marked as experimental, it's also fine to keep them.