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

Add item group for sell sign #5321

Open
wants to merge 4 commits into
base: 2.x
Choose a base branch
from

Conversation

Wyrdix
Copy link

@Wyrdix Wyrdix commented Apr 23, 2023

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.

@pop4959
Copy link
Member

pop4959 commented Apr 23, 2023

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.

@Wyrdix
Copy link
Author

Wyrdix commented Apr 23, 2023

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

@Wyrdix
Copy link
Author

Wyrdix commented Apr 27, 2023

@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

@pop4959
Copy link
Member

pop4959 commented Apr 27, 2023

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 pop4959 added type: enhancement Features and feature requests. module: main Issues or PRs for the main Essentials module labels May 5, 2023
@imDaniX
Copy link
Contributor

imDaniX commented Jun 19, 2023

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.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
module: main Issues or PRs for the main Essentials module type: enhancement Features and feature requests.
Projects
None yet
Development

Successfully merging this pull request may close these issues.

4 participants