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

remove duplication #34

Closed
wants to merge 2 commits into from
Closed

remove duplication #34

wants to merge 2 commits into from

Conversation

dominictarr
Copy link
Contributor

@mixmix I want to suggest this change. when I merged your earlier PR, I changed something: #31

I noticed that utils/get-net and utils/get-ws where nearly the same except that get-net iterated over all net protocols, but get-ws only took the first one. I figured surely this was an oversight - one of the problems we have with configuration is making sure things are handled in a consistent logical way, so the port that is being used for ws and net ought to be detected in the same way.

But, on the other hand, less abstract code is easier to read, because you just read it top to bottom...

But on the other hand, when things are the same, if they use the same code, then that strongly emphasizes that those things are the same. I think that makes it easier to maintain, because you can't change one thing without changing the other too. That way things intended to be the same change together.

What do you think?

@mixmix
Copy link
Member

mixmix commented Jan 21, 2019 via email

@dominictarr
Copy link
Contributor Author

@mixmix prior to this ssb-server worked without a default port set. because the port is defaulted to somewhere else.

I think the removing duplication aspect is more important: getting ws port should be done the same way as getting the net port, deduplicating code gaurantees that. duplicating code makes it easy to do it differently by accident

@dominictarr
Copy link
Contributor Author

@mixmix local should be synonym for private. actually "local" is preferred by secret-stack
https://github.com/ssbc/secret-stack/blob/master/core.js#L87

"private" is weird in that it means a local network... it might not be very private if it's it's say a cafe. 'public' is also unfortunate. but other code already expects "local"

@stale
Copy link

stale bot commented May 5, 2019

Is this still relevant? If so, what is blocking it? Is there anything you can do to help move it forward?

@stale stale bot added the stale label May 5, 2019
@mixmix
Copy link
Member

mixmix commented Nov 3, 2019

@dominictarr is this still relevant? Also, seems like the definition of local / private have evolved? Or is there a mixup here?

@stale stale bot removed the stale label Nov 3, 2019
@dominictarr dominictarr closed this Nov 4, 2019
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

2 participants