More types#297
Conversation
LiteApplication
force-pushed
the
more-types
branch
2 times, most recently
from
9936baf to
b5a9cd5
Compare
![qlty-cloud-legacy[bot]](https://avatars.githubusercontent.com/in/9403?s=60&v=4){kind=small}
LiteApplication
force-pushed
the
more-types
branch
from
b5a9cd5 to
124ecd7
Compare
LiteApplication
force-pushed
the
more-types
branch
2 times, most recently
from
c00916f to
9d40719
Compare
LiteApplication
force-pushed
the
more-types
branch
from
9d40719 to
80811f7
Compare
LiteApplication
force-pushed
the
more-types
branch
from
80811f7 to
65ff6ad
Compare
LiteApplication
force-pushed
the
more-types
branch
from
65ff6ad to
96d739c
Compare
ItsDrike
left a comment
ItsDrike
left a comment
There was a problem hiding this comment.
Thanks for putting so much work into the lib! I really appreciate it.
I have managed to finally go through most of this and review the code, though I haven't yet looked into the entities and codegen script. As there were a fair few things in the other manually written types that I found and I didn't want to wait with submitting the review until I look over all of the entity stuff too.
Hopefully I'll manage to get through that some time soon too, but for now, here's your task list haha.
|
Thank you for the review, I'm about to commit my changes but should I add the new types I wrote for the packets ? The current PR contains everything listed on the top of the "Current protocol" page, but there are some types missing that I noticed and implemented later. |
|
Oh, well, if the types are needed by some packets then yeah add them, if not, why are they even useful for us? |
|
They are defined later in the page, or on other page, it's a bit of a mess (like the whole brigadier for example, or the Trade format, the crafting recipe type, ...) |
Don't worry buddy, it's gonna be alright. |
|
Marking as draft to give me time to use |
ItsDrike
left a comment
ItsDrike
left a comment
There was a problem hiding this comment.
Had some time, so I reviewed some more.
| :type uuid: :class:`~mcproto.types.UUID` | ||
| :param username: The username of the connecting player/client. | ||
| :type username: str |
There was a problem hiding this comment.
The type should be picked up from the type-hint by sphinx autodoc, it shouldn't be necessary to doc it manually.
There's a lot of these in the code, this suggestion applies to all such instances.
(just use grep/rg to find all of them (or awk/tr like a chad) (or your editor's autoreplace, like a non-chad))
| :type uuid: :class:`~mcproto.types.UUID` | |
| :param username: The username of the connecting player/client. | |
| :type username: str | |
| :param username: The username of the connecting player/client. |
There was a problem hiding this comment.
They add the type between parenthesis (and links to the actual type) along with saying if it is optional :
Without :type ...: ...
Parameters:
- item – The item ID of the item in the slot.
- num – The count of items in the slot.
- nbt – The NBT data of the item in the slot, None if there is no item or no NBT data.
With it :
Parameters:
- item (int) – The item ID of the item in the slot.
- num (int) – The count of items in the slot.
- nbt (NBTag, optional) – The NBT data of the item in the slot, None if there is no item or no NBT data.
Also these type informations only appear on NamedTuples (without the helpful description next to them), and don't appear at all for regular Serializables.
There was a problem hiding this comment.
hmm, I think this might be configurable with autosphinx, will need to check though
|
Thing left to do before merging :
|
LiteApplication
force-pushed
the
more-types
branch
from
8e6981c to
2737072
Compare
ItsDrike
added
t: feature
|
I didn't manage to check the NBT for myself in this particular case but I was able to extract some packets and verify that they do not include the name tag, as stated in the documentation. |
LiteApplication
force-pushed
the
more-types
branch
from
5765bbf to
71cc9cd
Compare
|
I re-ran the tests with basedpyright, everything seems fine |
|
Shouldn't this get merged ? |
* Angle * Bitset and FixedBitset * Position * Vec3 * Quaternion * Slot * Identifier * TextComponent - Rename ChatMessage to JSONTextComponent
* Modified the conf.py sphinx configuration file to ignore the custom fields used in the Entity Metadata type * Added the Entity Metadata type to the documentation * Added `scripts` to the .codeclimate.yml ignore list * Added a changelog entry for the pull request
- Added sphinx build for the types - Use None as the default nbt value for Slot - Add even more types and tests
- Cache the classes for FixedBitset - Create an `AdvancementFrame` IntEnum - Use attrs' validators and convertors
when validating a single variable Fix some linting problems
- Update particle IDs in Particle Data (we really need that Registry stuff) - Add tests for ParticleData - Split Masked proxy EME into BoolMasked and IntMasked for improved speed, simplicity and type checking
Source : Conventions Fix one minor `basedpyright` issue
LiteApplication
force-pushed
the
more-types
branch
from
71cc9cd to
ddd8428
Compare
There was a problem hiding this comment.
Sorry the review took this long, I was pretty busy with other things recently, but mostly scared/too lazy to look at this much code lol. I've now gone over all of the files in the PR (finally), so these changes should be the last requested changes that need addressing, the rest looks good.
Also, a rebase with main should fix the CI failures.
| _NAMESPACE_REGEX = re.compile(r"^[a-z0-9\.\-_]+$") | ||
| _PATH_REGEX = re.compile(r"^[a-z0-9\.\-_/]+$") |
There was a problem hiding this comment.
Even though attrs won't consider these as instance variables, because there isn't a type-hint specified, the recommended practice for class vars is to annotate them with typing.ClassVar, you don't have to specify the generic type here, pyright can infer that, but do add the : ClassVar hint here.
| _NAMESPACE_REGEX = re.compile(r"^[a-z0-9\.\-_]+$") | |
| _PATH_REGEX = re.compile(r"^[a-z0-9\.\-_/]+$") | |
| _NAMESPACE_REGEX: ClassVar = re.compile(r"^[a-z0-9\.\-_]+$") | |
| _PATH_REGEX: ClassVar = re.compile(r"^[a-z0-9\.\-_/]+$") |
| from collections.abc import Iterator, Mapping, Sequence | ||
| from enum import IntEnum | ||
| from typing import ClassVar, Protocol, Union, cast, final, runtime_checkable | ||
| from typing import ClassVar, Hashable, Protocol, Union, cast, final, runtime_checkable |
There was a problem hiding this comment.
Huh, I didn't even know typing was re-exporting collections.abc.Hashable, it's not a generic type and it is available in 3.8, so there's no reason not to use the proper collections.abc version here.
| def read_header(cls, buf: Buffer, return_type: Literal[False] = False) -> int: ... | ||
|
|
||
| @classmethod | ||
| def read_header(cls, buf: Buffer, return_type: bool = False) -> tuple[int, int] | int: |
There was a problem hiding this comment.
Shouldn't this function be private? Same thing for read_value.
| @staticmethod | ||
| def finite_validator(instance: Vec3, attribute: Attribute[float], value: float) -> None: | ||
| """Validate that the quaternion components are finite.""" | ||
| if not math.isfinite(value): | ||
| raise ValueError(f"Quaternion components must be finite, got {value!r}") | ||
|
|
||
| x: float = field(validator=[validators.instance_of((float, int)), finite_validator.__get__(object)]) | ||
| y: float = field(validator=[validators.instance_of((float, int)), finite_validator.__get__(object)]) | ||
| z: float = field(validator=[validators.instance_of((float, int)), finite_validator.__get__(object)]) |
There was a problem hiding this comment.
What's the point of that finite_validator.__get__(object)? From my quick testing, it seems that you can just use finite_validator on it's own and it will work just fine.
Same remark for the Quaternion class which also does this, and any other occurences of this which I might've missed.
There was a problem hiding this comment.
Because it is a static method, not a regular function, in python 3.8 the __get__ part is needed to get a function because we are getting it from inside the class at creation time.
There was a problem hiding this comment.
Yeah I figured it's to get the function, I didn't realize it didn't work on 3.8 though, okay then but do add a comment explaining this in all occurrences of doing this.
Alternatively, I wouldn't even mind moving the functions outside of the classes.
There was a problem hiding this comment.
Rename this file to _entity_generator_data.py, just to better distinguish it from runnable scripts.
| def to_vec3(self) -> Vec3: | ||
| """Convert the vector to a Vec3. | ||
|
|
||
| This function creates a new Vec3 object with the same components. | ||
| """ | ||
| return Vec3(x=self.x, y=self.y, z=self.z) |
There was a problem hiding this comment.
Add some note to the docstring here that this function is primarily intended to be used by subclasses of Vec3 / Position class. It just looks really weird to see Vec3.to_vec3 function.
|
Ok great, I have been a little busy too. I'll try to make the changes soon ! |
py-mine-ci-bot
Bot
added
the
s: stale
ItsDrike
force-pushed
the
main
branch
2 times, most recently
from
2d364cc to
3f883fb
Compare
py-mine-ci-bot
Bot
removed
the
s: stale
py-mine-ci-bot
Bot
added
the
s: stale
2 participants
Depends on #285 New Serializable (v3)
All the types described in the list on Wiki.vg are now implemented and fully tested.