Add dependencies/references support

[libretto]

Protobuf references support:
Add references to Protobuf schemas,
Support compare protobuf schemas with references,
Support serialization/deserialization of protobuf schemas with dependencies.

[libretto]

There is some "copyright" lint check which is not passed on github but locally it is passed. Question is what this check means?

[RyanSkraba]

The copyright check is now in the pre-commit hooks: 1fd5ff7

I run this using pre-commit run --all-files.

It looks like the 3.11 failures are just enum types in error message and can be fixed (for all python versions): 's/{schema_type}/{schema_type.value}/g'

[libretto]

I run this using pre-commit run --all-files.
in my case pylint ignore copyright rules by some reason because locally I see
(py311) serge@dell:~/workspace/instaclustr/novage_office/karapace_mainstream$ pre-commit run --all-files
trim trailing whitespace.................................................Passed
fix end of files.........................................................Passed
debug statements (python)................................................Passed
copyright................................................................Passed

[libretto]

Debian 11 pre-commit rule not works probably because of this error:
(py311) serge@dell:~/workspace/instaclustr/novage_office/karapace_mainstream$ git grep -ELm1 'Copyright (c) 20[0-9]{2} Aiven' -- '.py' ':!init.py'
error: unknown switch `m'

[RyanSkraba]

Just some quick notes, so you know that we are looking at this!

I'm not asking for any changes yet -- I like to think a review is a collaboration and I still have quite a bit to get through. Don't hesitate to let me know if I've read or misunderstoon, and I'll keep on going through!

[libretto]

@RyanSkraba all issues above are fixed

[aiven-anton]

@libretto I logged a separate issue for the false positive in the copyright pre-commit hook: #563

[RyanSkraba]

Thanks for the merge with master -- I did this locally and it wasn't too difficult, but I appreciate that it's constant effort. I should have another round of review for you in the protobuf area very soon!

[RyanSkraba]

Hey,

I've finally gotten all of this code pretty thoroughly reviewed, and again, thanks for your patience! For me, this is a pretty good feature to ramp up on my Karapace and get some more modern python experience, so I appreciate it.

I really tried to limit myself to noting things that might be breaking changes, since the priority must be to avoid any regressions with customers not using this feature.

There's a couple of things introduced in this PR that could be improved, but I think it's wise to create issues and fix them incrementally! I'm willing to take on some of this work and I hope you'll stick around to help review future changes in this area :D

Just for discussion: some of the things that I would like to see applied on top of this PR:

• Restricting the scope as tight as possible helps reviews because we can tell the scope where a method applies.

• Some field, file and class names aren't very helpful. While this often applies to Karapace as a whole, we specifically end up with dependency.schema.schema constructions which is hard on the eyes.

• and of course, more extensive unit testing to catch regressions earlier.

To be clear, I'm not looking for the magic PR that makes everybody perfectly happy today! If we can do a round-trip on these comments, that might be enough to allow the above things to be gradually fixed while providing the references feature to those looking forward to it immediately.

all, What do you think?

[libretto]

@RyanSkraba I fixed all issues from Your comments. Most tests I ported from Wire (a java protobuf parser) or inspired by the code of Schema Registry. Of course, they do not cover all issues. You show an issue with an empty package name where one letter name will raise an exception. I will add there a simple test for one-letter names.

[RyanSkraba]

Hello! Thanks again for doing yet another merge with master -- it looks like there were a couple of hiccups with the merge with formatting and whitespace (which is now a lint error).

The good news is that I think that once the lint is fixed, this LGTM sufficiently to merge it and make any other requested fixes iteratively!

[RyanSkraba]

LGTM, thanks for your continued work!

[libretto]

all changes was implemented

[libretto]

@RyanSkraba I suppose we waiting for @jjaakola-aiven approval now...

[libretto]

@jjaakola-aiven I see "changes requested" in this thread. But all changes are done and all code comments are outdated. Could You take a look at Your change request and approve it?

[libretto]

All requested changes were fixed.

[RyanSkraba]

Thanks so much, all is well, I was doing some follow-up in the meantime -- let's merge this before main moves again! If we have any code style preferences, extra clean-up or fixes we can make those changes incrementally!

Some necessary changes can be made in future PRs