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

Provide http(s) default port method #347

Open
meistermeier opened this issue Feb 9, 2017 · 6 comments
Open

Provide http(s) default port method #347

meistermeier opened this issue Feb 9, 2017 · 6 comments

Comments

@meistermeier
Copy link
Contributor

It feels like a magic number to use 80 or 443 in the UriConfigurer#withPort method to suppress the port output given the right scheme.

There should be a method to set this magic number without user getting to see or having to use it like withHttpPort or withHttpsPort.

I know it is a small change but I think the right way is to discuss it before opening a PR.

@wilkinsona
Copy link
Member

How about withHttp() and withHttps() methods that set both the scheme and the port? They'd be shorthand for withScheme("http").withPort(80) and withScheme("https").withPort(443) respectively.

@meistermeier
Copy link
Contributor Author

I was thinking the same but was not sure if this might sound ambiguous (just the scheme or both) for the user. On the other hand this should be solved by a good documentation.

@wilkinsona
Copy link
Member

That's certainly a risk. Another option would be to deprecate withScheme(String) and withPort(int) in favour of:

  • withHttp()
  • withHttp(int)
  • withHttps()
  • withHttps(int)

The methods would set the scheme and the port. Methods that don't take an int set the port to the default for the scheme.

@meistermeier
Copy link
Contributor Author

That might be a better idea. Just one little thing: it could be confusing to have default port 8080 and switching it to 80 by using the withHttp() method. But in my opinion it is much better to provide this feature with a short and clear name and document it than having to pass numbers and strings 😁 to the configuration.

@wilkinsona
Copy link
Member

it could be confusing to have default port 8080 and switching it to 80 by using the withHttp() method

Hmm, I think that would be confusing. I think that puts that particular idea at the bottom of the list for me.

Another option would be for UriConfigurer to provide constants for the standard HTTP and HTTPS ports that can be passed to the existing withPort(int) method. I think that might be my preferred option at the moment as it doesn't introduce any new methods to the API and therefore hopefully doesn't introduce any possible confusion either.

@meistermeier
Copy link
Contributor Author

This simple improvement just points out how complicated it can be to introduce such a little change. I can understand that the pro and cons put this on the bottom on the list. (deleting local branch 😁 )

eddumelendez added a commit to eddumelendez/spring-restdocs that referenced this issue Feb 20, 2017
This commit adds new methods as a shorthand for the existing ones.
`withHttp` and `withHttps` allows to override the default port.

See spring-projectsgh-347
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Projects
None yet
Development

No branches or pull requests

2 participants