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

Equipment category schema rationalization #397

Open
MikkelPaulson opened this issue Jul 29, 2021 · 11 comments
Open

Equipment category schema rationalization #397

MikkelPaulson opened this issue Jul 29, 2021 · 11 comments

Comments

@MikkelPaulson
Copy link
Contributor

There's a general schema for links to other entities that looks like this:

"equipment_category": {
  "index": "armor",
  "name": "Armor",
  "url": "/api/equipment-categories/armor"
},

But most of the subcategories in 5e-SRD-Equipment.json look like this instead:

"armor_category": "Light",

The one exception to this is gear_category:

"gear_category": {
  "index": "standard-gear",
  "name": "Standard Gear",
  "url": "/api/equipment-categories/standard-gear"
},

My initial thought was that the categories that were represented as simple strings weren't found in 5e-SRD-Equipment-Categories.json, but nope:

{
  "index": "light-armor",
  "name": "Light Armor",
  "equipment": [
    {
      "index": "padded",
      "name": "Padded",
      "url": "/api/equipment/padded"
    },
    {
      "index": "leather",
      "name": "Leather",
      "url": "/api/equipment/leather"
    },
    {
      "index": "studded-leather",
      "name": "Studded Leather",
      "url": "/api/equipment/studded-leather"
    }
  ],
  "url": "/api/equipment-categories/light-armor"
},

I'm not sure what the policy is for breaking changes.

  1. Can I put up a PR to fix this?
  2. Since all of the subcategory types (gear_category, armor_category, vehicle_category, tool_category, weapon_category) appear at first glance to be mutually exclusive, can this be consolidated as an equipment_subcategory field?
@bagelbits
Copy link
Collaborator

I actually think that kind of makes sense? I'm not even sure where the concept of Standard Gear came from. It's not in the SRD at all.

We play a little fast and loose with breaking changes. Probably more than we should. If you want to do this right, I'd break this PR into two separate PR.

  1. Add the equipment_subcategory field
  2. Remove the extra category fields

This allows us to make a deprecation announcement between them and space out when we merge them.

@bagelbits
Copy link
Collaborator

To be clear, I think we should definitely do this.

@MikkelPaulson
Copy link
Contributor Author

I'll get there at some point if you (or someone else) doesn't beat me to it. I'm not working on integrating SRD data sets right now, so further data cleanups have been bumped to the back burner for me, but you'll know next time I do. 🙂

@bagelbits
Copy link
Collaborator

Sounds great! I'm sure someone will get to it!

@crazyfox55
Copy link

It appears that "arrow" equipment which might be a special case but it's in 3 different categories. "adventuring-gear", "ammunition", and "standard-gear". So I would actually suggest that "equipment_category" becomes a list instead of just adding a "equipment_subcategory".

@bagelbits
Copy link
Collaborator

@crazyfox55 That's an interesting suggestion. I think that makes a kind of sense. Like a list of tags. I'd love to see that in a PR.

@bagelbits
Copy link
Collaborator

@MikkelPaulson I had an idea related to this in another issue where it might make more sense to combine these like so:

  "equipment_categories": [{
    "index": "weapon",
    "name": "Weapon",
    "url": "/api/equipment-categories/weapon"
  }, {
    "index": "martial-melee-weapons",
    "name": "Martial Melee Weapons",
    "url": "/api/equipment-categories/martial-melee-weapons"
  }, {
    "index": "martial-weapons",
    "name": "Martial Weapons",
    "url": "/api/equipment-categories/martial-weapons"
  }, {
    "index": "melee-weapons",
    "name": "Melee Weapons",
    "url": "/api/equipment-categories/melee-weapons"
  }],

@mpeck451
Copy link

@MikkelPaulson I had an idea related to this in another issue where it might make more sense to combine these like so:

  "equipment_categories": [{
    "index": "weapon",
    "name": "Weapon",
    "url": "/api/equipment-categories/weapon"
  }, {
    "index": "martial-melee-weapons",
    "name": "Martial Melee Weapons",
    "url": "/api/equipment-categories/martial-melee-weapons"
  }, {
    "index": "martial-weapons",
    "name": "Martial Weapons",
    "url": "/api/equipment-categories/martial-weapons"
  }, {
    "index": "melee-weapons",
    "name": "Melee Weapons",
    "url": "/api/equipment-categories/melee-weapons"
  }],

@bagelbits If we did this, and we waited for a second PR to eliminate the old fields, then we'd have something like the following, right?

  {
    "index": "club",
    "name": "Club",
    "equipment_category": {
      "index": "weapon",
      "name": "Weapon",
      "url": "/api/equipment-categories/weapon"
    },
    "equipment_categories": [{
      "index": "weapon",
      "name": "Weapon",
      "url": "/api/equipment-categories/weapon"
    }, {
      "index": "simple-melee-weapons",
      "name": "Simple Melee Weapons",
      "url": "/api/equipment-categories/simple-melee-weapons"
    }, {
      "index": "simple-weapons",
      "name": "Simple Weapons",
      "url": "/api/equipment-categories/simple-weapons"
    }, {
      "index": "melee-weapons",
      "name": "Melee Weapons",
      "url": "/api/equipment-categories/melee-weapons"
    }],
    "weapon_category": "Simple",
    "weapon_range": "Melee",
    "category_range": "Simple Melee",
    "cost": {
      "quantity": 1,
      "unit": "sp"
    },
    "damage": {
      "damage_dice": "1d4",
      "damage_type": {
        "index": "bludgeoning",
        "name": "Bludgeoning",
        "url": "/api/damage-types/bludgeoning"
      }
    },
    "range": {
      "normal": 5
    },
    "weight": 2,
    "properties": [
      {
        "index": "light",
        "name": "Light",
        "url": "/api/weapon-properties/light"
      },
      {
        "index": "monk",
        "name": "Monk",
        "url": "/api/weapon-properties/monk"
      }
    ],
    "url": "/api/equipment/club"
  },

The initial PR would look pretty clunky with both an "equipment_category" and an "equipment_categories" field. Would we be okay with this for the initial PR?

@crazyfox55
Copy link

I think it's not that clunky it's like the primary equipment category and then the set of secondary categories. I have not thought of all of the downstream effects this change would have. Consumers of the data would all need to change to support a list of categories.

@mpeck451
Copy link

I agree this will be quite the change for consumers. We should definitely depreciate the old format first. As long as the above code looks acceptable, I think I'll get cracking on a build.

@bagelbits
Copy link
Collaborator

@mpeck451 I'm not sure your example removed any old fields. But that is what the interim state would look like. I assume the final result after the second PR would just be:

  {
    "index": "club",
    "name": "Club",
    "equipment_categories": [{
      "index": "weapon",
      "name": "Weapon",
      "url": "/api/equipment-categories/weapon"
    }, {
      "index": "simple-melee-weapons",
      "name": "Simple Melee Weapons",
      "url": "/api/equipment-categories/simple-melee-weapons"
    }, {
      "index": "simple-weapons",
      "name": "Simple Weapons",
      "url": "/api/equipment-categories/simple-weapons"
    }, {
      "index": "melee-weapons",
      "name": "Melee Weapons",
      "url": "/api/equipment-categories/melee-weapons"
    }],
    "cost": {
      "quantity": 1,
      "unit": "sp"
    },
    "damage": {
      "damage_dice": "1d4",
      "damage_type": {
        "index": "bludgeoning",
        "name": "Bludgeoning",
        "url": "/api/damage-types/bludgeoning"
      }
    },
    "range": {
      "normal": 5
    },
    "weight": 2,
    "properties": [
      {
        "index": "light",
        "name": "Light",
        "url": "/api/weapon-properties/light"
      },
      {
        "index": "monk",
        "name": "Monk",
        "url": "/api/weapon-properties/monk"
      }
    ],
    "url": "/api/equipment/club"
  },

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Projects
None yet
Development

No branches or pull requests

4 participants