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

Fix Active Item Cooldowns and Range #48

Open
wants to merge 2 commits into
base: master
Choose a base branch
from

Conversation

Galvantua
Copy link
Contributor

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.

@jjmaldonis
Copy link
Member

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.

@Galvantua
Copy link
Contributor Author

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 null, which no one can actively use anyways.

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.

@Galvantua
Copy link
Contributor Author

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?).
This could allow us to server both versions, however I would think it would be more usable to allow the user to decide what to do with the values, whether to just look at float/int values (which is not very expensive to parse), or be able to use and display the text with extra information.

@jjmaldonis
Copy link
Member

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 float and int. We also don't want to add more fields to the JSON to cover the str cases.

If we were to go for perfectly accurate data, we would want the range and cooldown fields to be converted into an object with various subfields. That would be the ideal case, but there are two concerns: 1) backwards compatibility because the types would change from float/int to objects, and 2) the code becomes less maintainable because we are trying to identify all the nuances inside an object representation.

So for now let's keep the fields as float/int, and if you want to fork the repo to convert them to strings or if you want to create objects out of them then this repo will be a good starting point. If, eventually, you convert a bunch of the int/float/str fields into objects and the overall data is much more accurate, and you'd like to submit a PR with those changes across the board, that would be awesome. We'd love to have more accurate data, and if it is maintainable then all the better. (Maintainability is a big thing because we don't have time to do the updates right now, so we rely on other people contributing.)

If there is a way to improve the quality of the ints/floats that would be great as well.

@Galvantua
Copy link
Contributor Author

Galvantua commented Dec 6, 2022

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".
For cooldowns, if we can strip the number out of the text field then that works just as well, the main fix here is that the regex used in the current version is old, and doesnt quite work anymore

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.

2 participants