-
Notifications
You must be signed in to change notification settings - Fork 4
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
Splitting up oi_inclusive_mobility() function #129
Comments
To do if a function ceases to exist in its current form:
|
I think splitting things into modular components is a good thing, 👍 to this! |
Yes, I am a big advocate for splitting this into multiple sub-functions! Additionally, for clarity:
The value of "maybe" implies that there is a lack of data to distinguish the value with any accuracy as being "yes" or "no" - so perhaps I should change this to "unknown" or "missing data" |
Splitting IM function into 5 separate functions #129
104b508 has achieved this. |
This is a follow up to my comment on #116.
I've noticed that some of the stuff James and I have done (e.g., presence of lighting) have started to overlap. For example, oi_inclusive_mobility returns lit column as it's important for inclusive spaces, but James created a separate function for it as well. This, I believe creates some redundancy and unnecessary confusion as the current amount of OSM data on lighting does not permit saying more than that the lighting is present, not, or maybe.
Thus, I'm wondering if it would make sense to split up oi_inclusive_mobility(), so instead of having a function that returns 6 columns in bunch, we would have a function for each column. If not, then I guess we need to make it clear in the documentation why oi_inclusive_mobility() returns lit column as well as why we have, e.g., oi_lit() function and that both basically do the same (James include "maybe" option but I do not as basically I treat everything as "non-existing" unless it can be deduced with certainty that a feature exists).
Any thoughts @hulsiejames, @Robinlovelace?
The text was updated successfully, but these errors were encountered: