add impl

[zargot]

No description provided.

[yulric]

Only took a look at the rules.py file for now since that's the one which has the only completed function (?). Only one comment. I also added docstrings to everything.

[yulric]

Should this be filepath? It looks you're setting it to the schema_path argument.

[zargot]

good catch, fixed by using the actual filename

[zargot]

Only took a look at the rules.py file for now since that's the one which has the only completed function (?). Only one comment. I also added docstrings to everything.

Thanks for the review.

I removed most of the docstrings you added, since they mostly seemed to be AI-generated boilerplate with a lot of confusing info, which also made it much harder to read the actual code. It's also in a private module, which we won't be generating documentation for.

I've retained a more complete docstring on the load function since it's the main function of the module.

When we're adding docstrings to the public modules I'd prefer to make them small and concise, and to use the Sphinx RST format. Sphinx should also be able to infer the types from our type hints.

[yulric]

Only took a look at the rules.py file for now since that's the one which has the only completed function (?). Only one comment. I also added docstrings to everything.

Thanks for the review.

I removed most of the docstrings you added, since they mostly seemed to be AI-generated boilerplate with a lot of confusing info, which also made it much harder to read the actual code. It's also in a private module, which we won't be generating documentation for.

I disagree re private functions should not have docstrings. They may not matter to a user of the library but as a developer of the library I find them very helpful. Otherwise I'm not sure what the function is meant to be doing.

I've retained a more complete docstring on the load function since it's the main function of the module.

When we're adding docstrings to the public modules I'd prefer to make them small and concise, and to use the Sphinx RST format. Sphinx should also be able to infer the types from our type hints.

Small and concise does not read well IMO. Documentation should be written in human-readable language so that its easier to understand, I find that short/terse sentences puts burden on the reader.

The Sphinx text makes sense.

[zargot]

I disagree re private functions should not have docstrings. They may not matter to a user of the library but as a developer of the library I find them very helpful. Otherwise I'm not sure what the function is meant to be doing.

You're right that docstrings can be good if they add necessary information, however, the function/parameter names and type hints should already give most of that information IMO, and it becomes very difficult to see the context of the code when every private function starts with 10-20 lines of documentation. Documentation in a public API file, on the other hand, doesn't interfere with the code in the same way.

Small and concise does not read well IMO. Documentation should be written in human-readable language so that its easier to understand, I find that short/terse sentences puts burden on the reader.

I think it's important to keep everything DRY and information dense. The shortest sentence to describe something, while including all the necessary info, would be the best sentence, IMO. In the case of a public API it would be important to explain everything the user needs to know, and more text can be used, since it's meant as more of a specification, but in our private module code I think it's extra important to not repeat ourselves and be concise.

[zargot]

I'm making sure to add a short docstring to private functions (that need it) from now on, as I can see the value it provides when doing a cold reading, but I'd like to avoid it taking up more than one line/sentence per function.

[yulric]

1. We should document the type of errors that a function can raise since that's not available in the function signature.

[yulric]

This should document the type of type_class since it does not have a type.

[zargot]

done

[zargot]

1. We should document the type of errors that a function can raise since that's not available in the function signature.

Done. I was only raising a single error type, and it always happened in fail/gen_error, which is why I just wrote "or fails", since failure always meant to raise that error. It also didn't depend on the name of the error, so it could make it easier to maintain. However, I agree that it can be good to see the error type names in the docstrings, and I also forgot to include one other error type. It should be good now.

[zargot]

waiting for the rest of the implementation before merge.