Skip to content
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

Added NBT type #257

Merged
merged 3 commits into from
May 14, 2024
Merged

Added NBT type #257

merged 3 commits into from
May 14, 2024

Conversation

LiteApplication
Copy link
Contributor

The NBT class has the following capabilities

  • Reading from Buffer object
  • Writing to Buffer object
  • Converting from/to python objects

100% test coverage for the nbt.py, Pyright and Ruff are happy

@ItsDrike
Copy link
Member

ItsDrike commented Apr 27, 2024

Hi, thanks for the contribution! Just make sure to add a changelog fragment file (see changes/README.md) and it seems that you're failing unit-tests CI on windows, due to ValueError: the environment variable is longer than 32767 characters. Once that's addressed, I will give this a more thorough look.

Btw, you can ignore the readthedocs check failing, it shouldn't be related to this PR.
Edit: the readthedocs issue was fixed in #258, you can update this (rebase/merge main branch) to pass the CI.

@ItsDrike ItsDrike added p: 2 - normal Normal priority t: feature New request or feature a: protocol Related to underlying networking protocol (connections, buffers, readers/writers, type classes) labels Apr 27, 2024
@LiteApplication LiteApplication force-pushed the main branch 6 times, most recently from f079cf3 to 60bec83 Compare April 27, 2024 22:04
@LiteApplication
Copy link
Contributor Author

The issue has been addressed, all the tests pass on all the platforms.

@ItsDrike ItsDrike added the s: needs review Author is waiting for someone to approve an issue/review a PR label Apr 27, 2024
@LiteApplication LiteApplication deleted the branch py-mine:main April 29, 2024 15:09
@LiteApplication LiteApplication deleted the main branch April 29, 2024 15:09
@LiteApplication LiteApplication restored the main branch April 29, 2024 15:11
Copy link
Member

@ItsDrike ItsDrike left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Note: I haven't yet checked the unit-test implementation at all, and this is more of a preliminary review, I haven't taken a deeper look at all of the logic and how things interract, but I wanted to submit it already since there's a fair few things I've noticed,

it's mostly minor fixes / standardization (surprised some of these weren't caught by the linter rules, I might need to take a look at that). But there are some more of a question based comments, where I'd like some feedback on how you meant for something to work, or on some code design choices.

PS: I know I'm a bit too pedantic, that's just how I am.

mcproto/types/nbt.py Outdated Show resolved Hide resolved
mcproto/types/nbt.py Outdated Show resolved Hide resolved
mcproto/types/nbt.py Outdated Show resolved Hide resolved
mcproto/types/nbt.py Outdated Show resolved Hide resolved
mcproto/types/nbt.py Show resolved Hide resolved
mcproto/types/nbt.py Outdated Show resolved Hide resolved
mcproto/types/nbt.py Outdated Show resolved Hide resolved
mcproto/types/nbt.py Outdated Show resolved Hide resolved
mcproto/types/nbt.py Outdated Show resolved Hide resolved
mcproto/types/nbt.py Outdated Show resolved Hide resolved
@ItsDrike
Copy link
Member

ItsDrike commented Apr 29, 2024

Note: this will need rebasing, as #131 introduced an additional pyright rule, which your current code is almost certainly in violation of, also pyright was (finally) updated in #264. Additionally #274 has dropped the use of black and isort in favor of ruff, and ruff was also updated to a much more recent version.

That all very likely means that you will need to do some extra work to pass validation worfklow after rebase.

@LiteApplication LiteApplication force-pushed the main branch 4 times, most recently from 638f6d5 to afa61ec Compare April 30, 2024 13:34
Copy link
Member

@ItsDrike ItsDrike left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Another quick one, just some more things I've noticed.

mcproto/types/nbt.py Outdated Show resolved Hide resolved
mcproto/types/nbt.py Show resolved Hide resolved
mcproto/types/nbt.py Outdated Show resolved Hide resolved
mcproto/types/nbt.py Outdated Show resolved Hide resolved
mcproto/types/nbt.py Outdated Show resolved Hide resolved
mcproto/types/nbt.py Outdated Show resolved Hide resolved
mcproto/types/nbt.py Outdated Show resolved Hide resolved
mcproto/types/nbt.py Outdated Show resolved Hide resolved
@LiteApplication LiteApplication force-pushed the main branch 2 times, most recently from 01b40e5 to 4427685 Compare April 30, 2024 18:25
 - Reading from Buffer object
 - Writing to Buffer object
 - Converting from/to python objects

 100% test coverage for the nbt.py, Pyright and Ruff are happy
 * Remove the TYPE class variable to rely in the actual type of each tag
 * Change the from_object and to_object to use schemas describing the data instead of using integer ranges and choosing types arbitrarily
Remove useless docstrings with @OverRide
Fix formatting for Sphinx docstrings
Return NotImplemented instead of raising the exception
Make use of StructFormat to read/write the numeric types
Copy link
Member

@ItsDrike ItsDrike left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

LGTM!

There are still some minor details that could probably be changed, but I don't want to block this with insisting on those. That can always be done later. Generally, the implementation looks good to me. Thanks!

@ItsDrike ItsDrike merged commit 8a1d934 into py-mine:main May 14, 2024
11 checks passed
ItsDrike added a commit that referenced this pull request May 14, 2024
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
a: protocol Related to underlying networking protocol (connections, buffers, readers/writers, type classes) p: 2 - normal Normal priority s: needs review Author is waiting for someone to approve an issue/review a PR t: feature New request or feature
Projects
None yet
Development

Successfully merging this pull request may close these issues.

2 participants