Skip to content

Comments

Let local_port default to remote_port#3

Open
ShaneDrury wants to merge 1 commit intomasterfrom
feature/default-to-remote-port
Open

Let local_port default to remote_port#3
ShaneDrury wants to merge 1 commit intomasterfrom
feature/default-to-remote-port

Conversation

@ShaneDrury
Copy link
Collaborator

Default to remote_port when local_port is not defined in the tunnel config.

The reason I do not put this rule in the TunnelerConfigParser is that it seems that object should be almost a one-to-one representation of the config file as it is on disk. The rule is then applied when we instantiate Tunneler which is the domain object where business rules should be.
Possible improvements could be to config.copy() instead of mutating the original config object. This would distinguish between config-as-on-disk and config-with-rules-applied.
We could also move the handling of configuration up a level and use it to drive the creation of the Tunneler object.

@ShaneDrury
Copy link
Collaborator Author

Good to merge? I've been using this locally and it seems to work well.

@judy2k
Copy link
Collaborator

judy2k commented Jan 15, 2016

Pedantically, I'm not sure I like the mutation of the config. If the config is a representation of the file that was loaded, then modifying it after loading from the file seems wrong.

I would normally implement this as some sort of wrapper around the config - as a property or something similar, so on access, you'd be given the configured or default value.

But... this project doesn't have a large codebase, and the feature seems like a really good idea, and I don't think this code is really going to lead to confusion. I also don't think that adding this single small feature should lead to a large refactoring of the config code. So 👍 from me.

@xoliver
Copy link
Owner

xoliver commented Jan 21, 2016

Sorry for not having replied. I quite agree with @judy2k and I reckon it could be done without much refactoring, right after the config parser class has loaded data for a given tunnel.

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.

3 participants