botblock / BotBlock.org

BotBlock - The List of Discord Bot Lists and Services
https://botblock.org/
Mozilla Public License 2.0
28 stars 8 forks source link

Improve field detection for get bot route #84

Closed cheesycod closed 3 years ago

cheesycod commented 3 years ago

This might be a bad PR but I feel this makes the API a bit easier to use as you can quickly see that’s it’s null. Using 0000 as discriminator may not immediately obvious

MattIPv4 commented 3 years ago

I don't see why we're changing the default values here, but I do see a few reasons for not:

  1. Using 0000 as discriminator may not immediately obvious. No bot can have 0000, so it is incredibly obvious.
  2. No idea why you've changed the default username being returned, this is a breaking change for zero benefit.
  3. Returning null is a major breaking change to how the API functions, as all the fields are being made nullable instead of returning a consistent type.

That being said, I appreciate the updates to the actual logic for parsing data from lists.

cheesycod commented 3 years ago

Yeah, it should probably stay an empty string then

cheesycod commented 3 years ago

@MattIPv4 maybe instead of doing this sort of parsing, it might be better to ask bot lists what keys they use and use that so u directly know what keys u need

MattIPv4 commented 3 years ago

I don't really think its viable or scaleable to try tracking all the potential fields from every list that we need to parse. Just having robust parsing logic that attempts to find everything is far easier, especially as not only the field names but also the data structures can vary.

MattIPv4 commented 3 years ago

You've still got some breaking changes in the defaults returned by the API which I'd like reverted please, there isn't reason to change the defaults.

cheesycod commented 3 years ago

Fixed