-
Notifications
You must be signed in to change notification settings - Fork 3
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
Restructure the project for new versioning approach #116
Conversation
4fa3568
to
df998f0
Compare
df998f0
to
d15ee35
Compare
In issue #45, it was decided that mcproto should not end up supporting multiple protocol versions, and instead it should only deal with the single (latest) version. This commit therefore restructures the project, removing all versioned components, keeping only a single version. This change also means a removal of the VersionMap ABC, which was used for PACKET_MAP construction for the various game states. As this is no longer the case, the current approach for now will be to construct the packet maps manually, which is indeed very annoying, and a better approach will need to be implemented later.
d15ee35
to
40ae446
Compare
Before this change, if the user called `generate_packet_map`, a dict object would get cached, and that same object (reference) would be actually returned. This means that when the fucntion would get called again, the same cached dict would be returned. Generally, this wouldn't be an issue, however if the user modifies this returned dictionary, since caching only stores the very same reference, on next call the modified version of that dict would be returned again. This was mentioned in the function's docstring as a warning, however I don't see that as sufficient, as people will usually do stupid things with libraries, without reading the docs, which in this case, would potentially end up breaking a lot of things. To avoid this issue, another decorrator was created, responsible for copying the returned object after the cache decorator. That means the cached version will hold a dict that no user will be able to modify accidentally, as the returned dict will simply be a copy of it. This does mean using slightly more memory, but it's a pretty small amount, and it's well worth the potential mess-ups this prevents.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
That looks good to me, but that last point bugs me a little.
- I like the idea of importing packets dynamically, as maintaining a big dictionary of packets does seem like a pain.
- I don't mind the memory overhead caching gives the library, because importing is a slow procedure.
- Is it possible to return some kind of "immutable dictionary", so that you don't have to copy the dictionary? Maybe
MappingProxyType
from thetypes
module? (available since Python 3.3)
This is a nice idea, however you need the real dictionary to be kept somewhere, and since a function call creates a new stackframe, with all of the variables in it, as the function ends, that stackframe gets deleted, and that means taking all of the local variables it held alongside. A mapping proxy needs a reference to an actual dict object, so we can't have that dict going out of scope, while the mapping proxy is returned. You could probably do something hacky, like crating a global variable, holding a list of dictionaries, which are the return values, so that the caching then returns that mapping proxy back, with the actual dict being in that global variable list. But this is probably way too convoluted and I'm not sure I love that. I do also find this caching with copying pretty annoying though, I just wasn't able to think of anything better. Btw, do you think the name |
Hmm, actually after some testing, it seems like this would work, as |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
This is a nice idea, however you need the real dictionary to be kept somewhere, and since a function call creates a new stackframe, with all of the variables in it, as the function ends, that stackframe gets deleted, and that means taking all of the local variables it held alongside. A mapping proxy needs a reference to an actual dict object, so we can't have that dict going out of scope, while the mapping proxy is returned.
You could probably do something hacky, like crating a global variable, holding a list of dictionaries, which are the return values, so that the caching then returns that mapping proxy back, with the actual dict being in that global variable list. But this is probably way too convoluted and I'm not sure I love that.
Riiight. Oh well, copying is not that bad after all.
Btw, do you think the name
utils/decorators.py
works well here? Or can you think of a better name? I didn't want an overly specific name just for this copying logic, to avoid too many files in utils dir, but decorators might be a bit too broad. Not sure where else to put it / what to call that file.
I can't think of a better name than utils/decorators.py
, so I guess it's final.
I'll approve the changes, then.
This reverts commit af54ac1. Following a [suggested change][suggested-change], this moves to using immutable `MappingProxyType` as return values, bypassing the need for copying. [suggested-change](#116 (review))
This reverts commit af54ac1. Following a [suggested change](#116 (review)), this moves to using immutable `MappingProxyType` as return values, bypassing the need for copying.
adec604
to
1461e1c
Compare
This reverts commit af54ac1. Following a [suggested change](#116 (review)), this moves to using immutable `MappingProxyType` as return values, bypassing the need for copying.
1461e1c
to
5ef1c00
Compare
This reverts commit af54ac1. Following a review comment: #116 (review), this moves to using immutable `MappingProxyType` as return values, bypassing the need for copying.
5ef1c00
to
28a06f8
Compare
This PR resolves #45, and restructures the entire project to no longer support multiple minecraft protocol versions, moving to a single (latest) version model.
Note that this is a completely breaking change, and there will NOT be a deprecation period. This is because of how this change impacts the rest of the project development. As this project is still in pre-release stage, deprecation here would greatly slow down any further development here, forcing contributors to maintain both the temporary deprecation layer, and the new single-version one.
For more details on how this change will affect the project going forward, and why it is necessary, I'd suggest checking out #45, containing a detailed explanation.
I have updated the README examples which now use this new version, so I'd suggest checking that out before moving forward with trying out this version, or reviewing the code, as it demonstrates how this new approach will work. Note that when testing this version out, it will almost certainly break any existing code-bases using mcproto, checking the README changes and looking at the changelog fragment message should make it clear on how to update.
Some decisions I made here, which I find potentially worth considering during a review. I'm open to doing these differently:
generate_packet_map
function is dynamic - imports the appropriate libraries on function run, depending on the requested packets. The alternative here would be to simply hard-code packet maps dictionaries as constants. This could be done either in the newpacket_map.py
file, but also in the individual game state folders for the packets, such asmcproto.packets.login import CLIENTBOUND_MAP
.generate_packet_map
uses caching, to avoid needlessly re-importing the modules and walking over all of the packets again. The clear downside of this is the fact that it uses more memory, though that's almost irrelevant as packet maps are relatively small.generate_packet_map
, the cache would hold the same reference as the returned dict, potentially causing issues if the user modifies that returned dict.For this reason, I decided to also add another decorator, responsible for copying the result afterwards, making sure that the cache holds a different instance of the dictionary, and what users see is simply a copy of this instance, which they can modify or do whatever they wish with, without risking breaking future calls to this function.For this reason, the function will now only return a mapping proxy, which is immutable, and holds a strong reference to the dict internally (as suggested by @Martysh12).