Enhance is_uri and is_uri_reference validation

[smaye81]

This ports the validation logic from protovalidate-go for validating URIs and URI references. As a result, all conformance tests for 0.10.3 related to these validations are now passing.

This uses a single underscore for protected class members and a double-underscore for private class methods to use name-mangling. There are varying recommendations on how to structure these (for example, Effective Python recommends public members, some recommendations suggest a single-underscore for class methods, etc.).

The current pattern in protovalidate-python seems to use a single underscore for module-level functions, which we could certainly adopt for classes. I'm open to whatever is the established pattern that we should adopt.

[smaye81]

Guessing this is what was meant by the 'cel types in the function' comment here.

I'm open to structure this however. We could do something like the following (omitting code for brevity):

def validate_uri(string: celtypes.Value) -> celpy.Result:
    if not isinstance(string, celtypes.StringType):
        msg = "invalid argument, expected string"
        raise celpy.CELEvalError(msg)
    return celtypes.BoolType(_is_uri(string))


def _is_uri(string: str) -> bool:
    """is_uri validates whether string is a valid URI."""
    return Uri(str(string)).uri()

...

def make_extra_funcs(locale: str) -> dict[str, celpy.CELFunction]:
    ...
    return {
         ...,
        "isUri": validate_uri,
        "isUriRef": validate_uri_ref,
    }

[timostamm]

It looks like celpy.CELFunction is pretty flexible regarding arguments. I think this might work out without the extra layer. I'd go without the extra layer, and revisit if it causes problems.

[timostamm]

That's not a very helpful comment. Is there a reason not to use the same doc as the source implementation?

Suggested change

	"""is_uri validates whether string is a valid URI."""
	"""Returns true if the string is an email address, for example "[email protected]".
	URI is defined in the internet standard RFC 3986.
	Zone Identifiers in IPv6 address literals are supported (RFC 6874).
	"""

• Gives an example so it's immediately clear what this is about.
• References the specification.
• Explains noteworthy details (zone id).

It's helpful information for users and maintainers.

Same for all other functions.

[smaye81]

I was actually copying docs from the Go implementation (which are just as unhelpful), so we should prob update that too.

Fixed.

[timostamm]

Yes, should update.

The current version of is_ip raises an error if a version other than 4 or 6 is given. The reference implementation is documented with:

Version 0 means either 4 or 6. Passing a version other than 0, 4, or 6 always returns false.

We want to nail down the behavior in those edge cases. That's why the docs are important.

[smaye81]

Ah yep. Good catch. I will update that in the forthcoming PR for porting is_ip logic.

[timostamm]

Is there a reason not to use the same doc as the source implementation?

/**
 * Returns true if the string is a URI, for example "https://example.com/foo/bar?baz=quux#frag".
 *
 * URI is defined in the internet standard RFC 3986.
 * Zone Identifiers in IPv6 address literals are supported (RFC 6874).
 */

[smaye81]

For this I was following that PEP proposal:

The docstring is a phrase ending in a period. It prescribes the function or method’s effect as a command (“Do this”, “Return that”), not as a description; e.g. don’t write “Returns the pathname …”.

I suppose we could change this to say 'Return true if the string...', but since we're documenting return types in the Returns: section, that doesn't make a lot of sense. I don't think the comments across implementations should be verbatim since each language is going to have its own guidelines and styles. As long as it expresses the exact same information. For example, the full description below this line includes all the RFC info.

[timostamm]

Fair to follow the PEP on phrasing 👍

[timostamm]

Is it typical in Python to document arguments, return value, and exceptions like this?

[smaye81]

Yeah, from the PEP proposal you linked:

The docstring for a function or method should summarize its behavior and document its arguments, return value(s), side effects, exceptions raised, and restrictions on when it can be called (all if applicable). Optional arguments should be indicated. It should be documented whether keyword arguments are part of the interface.

There are a few styles for doing this. This one uses the Google style guide, which is also used in Effective Python.

[timostamm]

I see we're also using a similar style in validator.py.

We definitely want to follow best practices, but there's some leeway. Don't need to completely rewrite the docs for every port.

[smaye81]

Yep, fair enough. Will keep that in mind going forward.