Essentials icon indicating copy to clipboard operation
Essentials copied to clipboard

Add item group for sell sign

Open Wyrdix opened this issue 1 year ago • 5 comments

In reply to #5297

I didn't use the '+' mdcfe, I preferred to use '~' because it's more neutral Maybe we could make a configuration of it

To be clear, this is one of my first PRs, so feel free to point out any mistakes I made.

Wyrdix avatar Apr 23 '23 11:04 Wyrdix

I'm not sure it is a great idea to add yet another data file for this. We already have great difficulty in maintaining the items.yml and worth.yml files, as these largely need to be manually updated. With groups, there will also be some expectation that we maintain default groups, and if we don't, I don't see this being used by very many people (as this is a more niche use case, and can be solved generally with multiple sell signs instead of one).

I think a better version of this uses tags, as md mentioned in the original thread linked. That way there is some standardization as to what groups exist, and with Minecraft steadily becoming more data driven with each update, more likely than not there will be tags for most things you would want to make groups for anyways.

pop4959 avatar Apr 23 '23 16:04 pop4959

Thanks for the feedback, I can't look this until Wednesday but I'll check it out

Wyrdix avatar Apr 23 '23 17:04 Wyrdix

@pop4959 I've added the possibility of using minecraft item tags, for now i didn't removed the possibility of creating custom groups in the config file. I don't mind deleting the files but I don't know if it's best to keep it or to remove it

Wyrdix avatar Apr 27 '23 16:04 Wyrdix

Would love to get some feedback from the rest of the team and community members first, but I see two options currently for this kind of feature:

  1. Everything is defined as groups and we bear the maintenance burden of updating these manually. Might warrant a data generator like we have for items so we can support various built-in and popular groupings and somewhat keep up with them more automatically.

  2. Keep things simple and only support groups that already exist as vanilla tags. This requires less maintenance on our part and alleviates some of the regrets we have with some of our old features which currently rot away since we can't sensibly maintain them ourselves (see for example: worth.yml).

Or maybe in-between, build in the defaults as vanilla tags and not maintain our own at all, but allow editing these (adding, removing, changing) via a config file. This may be most robust and prove to cause less long term headaches.

pop4959 avatar Apr 27 '23 20:04 pop4959

I'd like to see # here rather than ~, as the hash character is already used in the vanilla syntax for such groups.

Or maybe in-between, build in the defaults as vanilla tags and not maintain our own at all, but allow editing these (adding, removing, changing) via a config file. This may be most robust and prove to cause less long term headaches.

I think the plugin should generate groups file from vanilla tags (only if such file is absent), and then let the user decide what to do with it. Using vanilla tags directly is a bad idea - they do change from time to time, sometimes get removed, and so on - it would be a nightmare to update every sign on a server in such case. So if they want to sync their tags with latest MC version, they'll just delete their current groups file and let the plugin regenerate it. I also think the file name groups.yml sounds too abstract - some people may think it's somewhat related to permission groups, or home/list groups. item-groups.yml would be somewhat better.

One more thing that I want to point out is that there was no such concept of vanilla tags up until 1.13, so ancient version users will need to create such groups all by themselves. Also, because of that, I'm pretty sure the current implementation won't work on 1.12 and lower.

imDaniX avatar Jun 19 '23 13:06 imDaniX