Paper icon indicating copy to clipboard operation
Paper copied to clipboard

Add existsStatistic with Material and EntityType check

Open petomka opened this issue 1 year ago • 3 comments

When I was trying to modify some statistics in batch, I noticed that there wasn't a way to check if a certain statistic and material/entity type combination exists, instead the methods just threw exceptions whenever a certain combination was invalid.

petomka avatar Jul 30 '22 00:07 petomka

Rename 0389-Add-existsStatistic-with-Material-and-EntityType-che.patch to 0389-Add-existsStatistic-with-Material-and-EntityType-check.patch :)

patch file names are automatically generated based off of the commit and have a length limit

MiniDigger avatar Aug 09 '22 10:08 MiniDigger

I don't think this should be merged. I don't want to expand on this API, I want it replaced (See the existing Better Stats API PR).

The current stats API is just bad. its really that simple. No use expanding on a bad system.

it's easy to say "we should just use the new API" but considering that new API has been in PR hell for over a year now, i don't really see a good reason why not to merge this 🤷🏻‍♀️ this is a relatively minor and harmless PR; it just needs some cleaning up of the imports and some nullability annotations

qixils avatar Aug 10 '22 18:08 qixils

slapped an approval on the other PR, I'd forgotten I'd already given it a look over as for this one,

  1. maintaining extra API which is just deprecated on day 1 is pretty bleh, so not sure about that entirely
  2. if this was to be merged, that API method needs to be renamed
  3. needs cleanup

electronicboy avatar Aug 10 '22 18:08 electronicboy

Thank you for your contribution, however, as stated by Cat and Machine it's probably better to freeze any additions to this API due to its planned replacement.

I also agree with this, because this is just annoying maintenance for us on something we want to be replaced anyways.

Owen1212055 avatar Oct 22 '22 23:10 Owen1212055