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

Check if datatypes are reasonable #1467

Open
SimonHoenscheid opened this issue Aug 25, 2023 · 3 comments
Open

Check if datatypes are reasonable #1467

SimonHoenscheid opened this issue Aug 25, 2023 · 3 comments
Labels

Comments

@SimonHoenscheid
Copy link
Collaborator

Use Case

It seems like some datatypes could be more accurate

Describe the Solution You Would Like

correct datatypes if reasonable

Additional Context

example:

Variant[String[1], Stdlib::Port, Integer] $port = $postgresql::server::port,

Since Integer accepts all integers and String[1] all non empty strings the specialized Stdlib:Port will be without effect if it is a Pattern.
It does not make sense to have both Integer and Stdlib::Port since it is defined as Integer[0,65535] - should probably remove Integer from the variant unless it is possible to have a port that is > 65535.

@ekohl
Copy link
Collaborator

ekohl commented Aug 25, 2023

It does not make sense to have both Integer and Stdlib::Port since it is defined as Integer[0,65535] - should probably remove Integer from the variant unless it is possible to have a port that is > 65535.

Excellent example: it's impossible to have a port > 65535 because there simply aren't more bits in the TCP header. it's 16 bits and 2¹⁶ is 65536. If you start at 0 you end up with 65535 as the upper bound.

Similarly, we shouldn't accept strings for ports, unless the software allows specifying a range (like 10-20). Even then a Pattern would be better.

@SimonHoenscheid
Copy link
Collaborator Author

TIL! Thanks @ekohl

@SimonHoenscheid
Copy link
Collaborator Author

Removing Strings support for the $port parameter would be a breaking change. I am fine with that. Currently there are tests which cover this behaviour too.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
Projects
None yet
Development

No branches or pull requests

2 participants