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

Changes to behaviour of Item #41

Open
gilesknap opened this issue Aug 21, 2022 · 6 comments
Open

Changes to behaviour of Item #41

gilesknap opened this issue Aug 21, 2022 · 6 comments

Comments

@gilesknap
Copy link
Contributor

In the latest version of mcipc there have been some changes to behaviour of Item.

I could fix my code to work with the new behaviour but I'd like to understand if these are intentional.

# pip install mcipc==2.3.8 rcon==2.2.1

In [2]: str(Item.AIR)
Out[2]: 'minecraft:air'

In [3]: Item("minecraft:air")
ValueError: 'minecraft:air' is not a valid Item

In [4]: Item("air")
Out[4]: <Item.AIR: 'air'>

#pip install mcipc==2.4.0 rcon==2.3.1

In [2]: str(Item.AIR)
Out[2]: 'Item.AIR'

In [3]: Item("minecraft:air")
Out[3]: <Item.AIR: 'minecraft:air'>

In [4]: Item("air")
ValueError: 'air' is not a valid Item
@gilesknap
Copy link
Contributor Author

gilesknap commented Aug 21, 2022

My preferred behaviour would be that Item __str__ returns the short form but the constructor could take the long or short form:

In [2]: str(Item.AIR)
Out[2]: 'air'

In [4]: Item("air")
Out[4]: <Item.AIR: 'minecraft:air'>

In [4]: Item("minecraft:air")
Out[4]: <Item.AIR: 'minecraft:air'>

(I believe all commands are OK with the short form?)

@conqp
Copy link
Owner

conqp commented Aug 22, 2022

Yep. Unintentionally my last change to this Enum caused an unintended change of the previous behavior.
I reverted the changes and also added regression tests.
Fixed in 19a22f9

I made str(item) return the long form, since I think I had problems with the short form on some commands.

@conqp conqp closed this as completed Aug 22, 2022
@gilesknap
Copy link
Contributor Author

@conqp for some reason this is still breaking the mcwb modules tests.

There is a change in behaviour which is affecting the vaildate function https://github.com/mcipc-tools/mcwb/blob/master/mcwb/functions.py#L88-L101.

When casting an array of Item to a numpy array the numpy array now ends up containing str instead of Item and this breaks the validation. Its not clear to me why this is happening.

Investigating further.

@gilesknap
Copy link
Contributor Author

downgrading mcipc and rcon does fix the issue.

@conqp
Copy link
Owner

conqp commented Aug 27, 2022

Well, Item had a __str__ method before, returning the long form, so that kind of behaviour should not have changed.
However, Item now is also a subclass of str, which is intentional.

@conqp conqp reopened this Aug 27, 2022
@gilesknap
Copy link
Contributor Author

OK the subclass of str is probably what is doing it. numpy likes to use the base class of its elements as its type, I believe.
It also means that ndarray.toList() is returning a list of str even if you do make a numpy array of Item.

I have worked around this by supplying a dtype of Item to numpy constructors, so mcwb is working again. Now when we save an 'Items' object to a json file it now looks like a list of long form string (because it uses toList), but this works OK and is a bit more legible than the old format. loading of Items objects still supports the old and new json formats.

In short, I think the new behaviour is now fine for mcwb and mciwb. The only problem is if there are any other users relying on the old behaviour.

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

No branches or pull requests

2 participants