Skip to content

Conversation

@silasary
Copy link
Collaborator

No description provided.

@silasary silasary mentioned this pull request Oct 16, 2024
@FuzzyGamesOn
Copy link
Collaborator

FuzzyGamesOn commented Oct 18, 2024

Easy enough. We'll also need a related PR in Builder to support users importing this, since it's one of the fields that Builder does currently support.

@nicopop
Copy link
Contributor

nicopop commented Nov 18, 2024

Don't forget to add that to the json schema

nicopop
nicopop previously approved these changes Dec 10, 2024
@FuzzyGamesOn
Copy link
Collaborator

I can't approve this until there's a Builder PR to support the classification field, since using it instead of the existing classification keys would cause worlds to not import correctly to Builder.

@silasary
Copy link
Collaborator Author

silasary commented Jan 2, 2025

Weirdly enough, the builder already supports the classification field.

Internally, the builder uses item["classification"] instead of all the bools. And the way it loads the items, if that field is prepopulated, it'll just roll with the value.

A round trip through the builder converts the item to the old format, but it works well enough for my liking.

@FuzzyGamesOn
Copy link
Collaborator

It's awesome that it sorta supports it, though I somewhat disagree on the quality bar. But that's kinda irrelevant because I realized...

#105 added support for multiiple item classifications, which I believe was started and completed after this PR was started. So this PR would need to be updated to similarly support multiple item classifications to ensure that it can have the same information as the separate fields.

Also realized we didn't get #105 to update Builder to support it. So no reason I should require any level of support for this one, either. Just feature parity in core is fine.


I swear I'm not trying to find things to hold this PR up. Just unfortunate timing on this one 😅

@FuzzyGamesOn FuzzyGamesOn added the blocked by feedback This PR is blocked by feedback in the PR label Apr 17, 2025
@FuzzyGamesOn
Copy link
Collaborator

Ran back through this because of the label. Looks like it's good to run with once it supports multiple values, to be consistent with the current multiple classification support in separate keywords.

@FuzzyGamesOn FuzzyGamesOn removed the blocked by feedback This PR is blocked by feedback in the PR label Oct 16, 2025
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.

4 participants