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

Move some File functionality to an intermediate Root class #55

Merged
merged 3 commits into from
Mar 24, 2020

Conversation

MestreLion
Copy link
Contributor

As discussed in #1 :

Some NBT Compound tags are root tags but are not a file, namely Chunks from a Region File. As such it would be useful for them to have .root and .root_name but not .load(), .save(), etc.

This PR creates a Root class representing such compound tags, inheriting from Compound, move some of File functionality to this new class, and change File to inherit from Root

@MestreLion MestreLion changed the title Root Move some File functionality to an intermediate Root class Dec 20, 2019
@vberlier
Copy link
Owner

I like the idea of making the root property accessible on plain compound tags. Actually I think it would even make sense to just put it in the Compound class directly. This way you could be able to treat any compound tag as the root of an nbt document, kind of like the as_unsigned helper on integer tags.

@MestreLion
Copy link
Contributor Author

Not sure if it's a good idea to "pollute" Compound with methods and properties that make sense only to a subset of its instances. Also, I'm planning on some more intrusive changes to Root, such as completely hiding the unnamed root tag, as discussed with @ch-yx. If not done properly this may break Compound in subtle and hard to find ways, hence I postponed the idea until I get it right. So for now a separate class provide me the "hook" I need to make changes without affecting regular Compounds. Take a look at how I define _root_name on mcworldlib.Level and mcworldlib.Chunk

Besides, adding .root to Compound might raise the question: why not adding .save() too? And then .load(), and then .filename, and then...

You gotta draw the "it stops here" line somewhere :-)

All that said, I'm not saying I'm against this, I'm just taking a conservative approach until I think more about this.

@17183248569
Copy link

17183248569 commented Dec 25, 2019

Actually I think it would even make sense to just put it in the Compound class directly.

I dont think this a good idea too.

Not all Compoud has this meaning. putting all method in the same class is against OO?

@MestreLion
Copy link
Contributor Author

@vberlier : The main goal of this PR is to split File and Root. For Chunks this is really important.

You can move functionality from Root to Compound anytime later on (or move tag_id = 10 from Compound to Root, which effectively makes .parse() automatically generate Root instances instead of "plain" Compound - just an idea)

For now I really need a Compound subclass that has .root functionality without .save()/.load() file features, hence this PR. When done I can push additional PRs of RegionFile to the newly suggested contrib package, as hinted in #60

@vberlier
Copy link
Owner

Hey! I'm really sorry I had to take a break to work on other things... Anyway I just rebased this on master and reverted the change to __repr__ as I thought it wasn't necessarily needed. I'm merging it right away 👍

@vberlier vberlier merged commit e9b1281 into vberlier:master Mar 24, 2020
MestreLion referenced this pull request Oct 23, 2021
BREAKING CHANGE: The Root class is gone and so is the root attribute on files

Fixes #145
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

3 participants