-
Notifications
You must be signed in to change notification settings - Fork 25
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
Fix Active Item Cooldowns and Range #48
base: master
Are you sure you want to change the base?
Conversation
One thing is that we're pretty hesitant to change data types. It's perfectly fine to add a new field to the JSON, but changing a data type can immediately break people's production code for any typed language. For example, if Java gets an unexpected/incorrect type in the JSON, then the parsing fails and the data is not available to the rest of the code (or an exception is thrown). If any production apps rely on the fields where the data type was changed, this PR could take their app down immediately and with no warning. Is there a strong reason to change the data types? What problem is it solving? I'm also hesitant to change data types from float/int to str, because that effectively is requiring the user to perform the parsing that this library is supposed to be doing. We do our best to provide accurate and usable information, rather than just accurate information. If someone wants perfectly accurate information, they will need to go beyond this tool to get it, but this tool does provide usable (if sometimes inaccurate) information. So this is a tradeoff that we want to be careful about, and I'd like to better understand the pros and cons of changing these data types before doing so. |
Hey! Currently, several values in those fields are lost completely. Check the current value of active cooldowns, from what I could see they are all Currently "Global" range actives such as Anathema's are listed as 0, which is obviously incorrect, as well as other items containing wikitext which is lost. |
A separate solution I just thought of is implement the change to correctly grab the active cooldown, but continue parsing as float. THEN we could add a subsequent field (cooldownString?) that contains the full value (including wikitext), could add a similar field for range (rangeString?). |
We've been thinking about this. For now, we don't want to break backwards compatibility so the field types will need to stay as If we were to go for perfectly accurate data, we would want the So for now let's keep the fields as If there is a way to improve the quality of the ints/floats that would be great as well. |
Heya! thanks for the update. I've been thinking about how to improve the quality of the existing datatypes, and I'm going to make some commits and come back with some examples (mainly going to check if range of 0 exists, and if not, we can continue treating 0 as "Global". |
The old regex method of sourcing cooldowns from the wiki is no longer necessary, and has been replaced with a simple working version that grabs the cooldown as a string (to prevent issues of non-float values being replaces with null or 0).
Global range item actives show a range of 0, fixed by making range a string value, allowing the data to be stored.