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.
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
Add network library #299409
base: master
Are you sure you want to change the base?
Add network library #299409
Changes from all commits
ad9df7e
79dc1bc
33747ae
5730535
4cb8f2d
5a963ec
51de0d0
ee3d0f8
de10daa
5898ff8
f5ab94e
52eef00
0e3651b
ab3707a
e1f9b01
7e55177
5ea3344
6116326
3a09ebe
18e64c7
24c77da
f28ca5a
b7c93e1
a8ab014
61b60a3
ff6f0e2
c27b3d6
b871844
File filter
Filter by extension
Conversations
Jump to
There are no files selected for viewing
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I am not really familiar with the documentation syntax, but it looks like L19-21 content redundant with documentation of function below... Does it really need to be duplicated?
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Does this comment even get rendered?
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I think
fromCidrString
was not meant to be part of the above list.There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Shouldn't this return something like
[ 192 168 0 1 ]
, such that other functions can more easily work with the data instead of having to re-parse it?There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
After thinking about it, that is a good idea. But I think this should be stored in an internal variable like
_address
with the string representation stored inaddress
. The reason being that the general use case for the IP address is in it's string representation. But yeah having the internal representation as a list of ints is probably the best state to store it internally.There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Doesn't seem to be in the result.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
You sure? I just created a test that fails to check the
_parse
function and it definitely is in there.There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Similarly
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
This function doesn't actually do any parsing at all. Overall I think these internal functions could be refactored to this:
_parseIp :: String -> String -> Int
Example:
_parsePrefix :: String -> String -> Int
Example:
_parseCIDR :: String -> String -> { ip :: Int, prefix :: Int }
Splits and calls
_parseIp
and_parsePrefix
underneathAt this point, the public
fromCidrString
function can call_parseCIDR "lib.network.ipv4.fromCidrString"
to get back two integers that are guaranteed to be valid. If they're not you'll get failure that also indicates that function call failed (you can add extra context to the error messages as desired).All the internal functions for generating data now don't need to do any validation themselves.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I'm having a hard time understanding your suggestion. What is the first argument for these parse functions supposed to do?
For example, in
_parseIp "lib.network.ipv4.someFunction" "12.34.56.78"
, I don't understand what the intent of passing"lib.network.ipv4.someFunction"
and how it would be used in the function.There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
AFAICT, it seems to be for error messages to only mention public functions (
fromCidrString
) instead of private ones (_parseCIDR
). Using these, we can make sure users only get messages about the API of the library, and not implementation details.c.f.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Consider setting
_type = "ipv4Address";
for runtime type checking. This can improve error messages when addresses end up in wrong places; useful even if this library doesn't check_type
.There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
This also reminds me that a type in the module system would be nice (for later perhaps).
It could parse both stringly definitions and pass through
_type
-ed definitions.