-
-
Notifications
You must be signed in to change notification settings - Fork 348
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
Taginfo listing of tags added or modified by StreetComplete #4225
Comments
Not necessary.
Depends on how frequently this would need to be done / for how many we are talking about. No promises until I have seen how much manual work this is amounts to for current and future quests and overlays. |
Care to give some detail @matkoniecz . Obviously I spent quite a bit of time looking at it before in various iterations of the code.
FWIW that bit feels like it ought to be pretty trivial compared to the rest of it, you'd just need to parse the JSON for the presets and dump it out.
I'd agree with this and proposed similar in the past, there was some stuff like I think the cycleway left/right/both where there were just too many levels of indirection to do it in a sane way. It would also act as a stepping stone; what I'd generated before automatically picked up most new quests, and was already a lot better than the current very stale manual effort. If we covered 90% automatically and just hardcoded some, we could get something out there and then potentially one or more people could look at automating more of the 10% in future, but I suspect as is often the case the last 5/10% will be the hardest (e.g. looking up the name:en etc suffixes is a special case for one or two quests and very different to everything else). |
Parsing AST generated with https://github.com/kotlinx/ast
not really, as SC is using only part of them - you need to also filter them |
I think I said similar in my old PR, while it may deal with some of the whitespace issues the regexes had, you're still going to have to interpret the AST from even something relatively simple like: Lines 40 to 45 in 7c97885
or StreetComplete/app/src/main/java/de/westnordost/streetcomplete/quests/barrier_type/AddStileType.kt Lines 50 to 80 in 7c97885
Maybe the former isn't too bad, but for the latter, you need to know that Maybe it's worth outputting the AST from those two quests as a starter if you haven't already?
Still easier than the above, even if you have to code the filters in parallel! Hell you could probably just write some Kotlin that runs a for loop on osmfeatures and uses the exact same filter(s). |
Can we extract the information from the test infrastructure? |
How we would do this? |
Hack/hook into |
That would require manual creation of tests exercising use of every single value which would be both pointless. And would be both manual and requiring tricky code. |
Another "fun" corner case is semi-colon separated lists see #4230 ! |
The parsing code is filled with special cases and hardcoding, to the point that I am unsure about making a pull request. And more complex than I expected. If I would guess the complexity correctly at start, then I would skip it and definitely not treat it as a stepping stone to some noncritical improvements. My plan right now is to wait say half year and see how much effort is caused by StreetComplete development - is it requiring new complex tweaks all the time or is it stable. See https://github.com/matkoniecz/Zazolc/tree/taginfo and more specifically https://github.com/matkoniecz/Zazolc/blob/taginfo/buildSrc/src/main/java/UpdateTaginfoListingTask.kt and https://github.com/matkoniecz/Zazolc/blob/taginfo/res/documentation/taginfo_listing_of_tags_added_or_edited_by_StreetComplete.json It is deployed to taginfo as it is superior to what was deployed already - see https://taginfo.openstreetmap.org/projects/streetcomplete#tags and for example https://taginfo.openstreetmap.org/keys/addr:place#projects Feedback about code or about mistakes in produced listing is welcome. |
This one was relatively easy to handle
I gave up here and switched to partially hardcoding things https://github.com/matkoniecz/Zazolc/blob/31f1971bfc3a73b0ada60e7a3c33da9e2776390b/buildSrc/src/main/java/UpdateTaginfoListingTask.kt#L607-L632
works fine, there was no need for doing anything really special. Code in this case just notices that It makes few assumptions, for example that |
Overall it was fun to implement, but likely not worth invested time if you treat taginfo listing as the only beneficial output. |
Hmm, I have to admit that I do not particularly fancy reviewing a 2000 lines of code that are(after reading it vertically) full of debug logs, TODO comments, commented out code and 200+ lines of hardcoded/semi-hardcoded rules for quests. I can't tell how much of that code is just for generally traversing the AST and finding what one needs to know and what of it is workarounds, hardcoding, semi-hardcoding, debugging stuff etc, as it is all mashed in one script, so I cannot tell if it is worth maintaining in the future.
This is also not something I would like to review. It would mean going through the taginfo pages one by one and comparing wiht the quests, one by one, if the information is correct. This is something that should happen by the person who developed it, not by (code) reviewers |
For what it’s worth, I took a different approach when putting together a taginfo project file in osm-americana/openstreetmap-americana#179. That map renderer project’s interactions with raw OSM tags are largely isolated into a method that can be run “headlessly” as part of a mocked-up environment. Instead of outputting a map style or unit test results, it spits out the major portion of a project file. A separate “template” file contains anything else that needs to be mentioned in the template that isn’t as easy to isolate or that is obfuscated by OpenMapTiles. StreetComplete is a very different kind of application, but I would suggest finding a way to execute the code instead of scraping it. It looks like some quests already have automated tests that would be easier to turn into taginfo documentation. If a code coverage tool says test coverage of the quest code is good enough, then maybe figure out how to log something from methods like |
Overall I will close this issue, as I have no idea how to provide non-nightmarish solution that can implement it. I am planning to support what I created, so feel free to ping me here or at https://github.com/matkoniecz/Zazolc/issues if you spot anything wrong.
Yes, at this point I am really dubious about including it as a part of the official SC code. Maybe it would be better for it to live in a unofficial state like right now?
That is not true at all in SC
That would not work at all. For example https://github.com/streetcomplete/StreetComplete/blob/master/app/src/main/java/de/westnordost/streetcomplete/quests/building_type/AddBuildingType.kt#L58 may have many different values passed to it. And to trigger all values for recording - that would require manual creation of tests exercising use of every single value which would be both pointless. And would be both manual and requiring tricky code. And still would not deal with a freeform values.
Oh, I did it and found a bug (in hardcoded answer listing). Still, if anyone spots problem - lets me know. |
I understand, but even though the two projects greatly differ, my point was that piggybacking on running code is the only way to have certainty that the project file is consistent with the running code. Even with an AST, trying to interpret source code independently of the running application will inevitably lead to unmanageable edge cases in a nontrivial codebase.
If this code can set |
Possible values are listed in https://github.com/streetcomplete/StreetComplete/blob/master/app/src/main/java/de/westnordost/streetcomplete/quests/building_type/BuildingType.kt#L61 - then this list is used to build interface and selected value is passed in https://github.com/streetcomplete/StreetComplete/blob/master/app/src/main/java/de/westnordost/streetcomplete/quests/building_type/AddBuildingType.kt#L58 I see no way of piggybacking on running code that will work, but maybe someone will notice a way to do this. It is possible to write tests to trigger manually use of each value, but that is equivalent of making this tag list by hand.
In such cases I see no way to detect it by piggybacking on running code. While AST analysis can actually sort-of detect this.
Yes, part of my parser degenerated into hardcoding answers. |
Does Kotlin support iterating over the values of an enumeration? If so, a test could loop over it, logging out the values as being supported by the application. I realize this would be quite contrived as a test of the application’s actual functionality, so it’s a question of to what degree the project would accommodate test changes needed only for the taginfo project file.
Earlier, I suggested mocking the user input: a test could pass in an easily detectable bogus string as the tag value, which the test could then check for. Techniques like this have the advantage of improving test coverage at the same time as they’d make the taginfo project file more maintainable. Even though AST analysis is resilient to differences in formatting, it constrains the code structure so that developers have more rules they have to follow when contributing a quest, just for taginfo’s sake. But relying on code coverage is easier because there are tools for automatically measuring it. (Unfortunately, I can only casually suggest these strategies, since I’m not as familiar with Kotlin or this codebase, but hopefully a maintainable taginfo file doesn’t seem so hopeless.) |
OK, good point about such iteration. I am starting to see how one may get it to work. Though I am not planning on doing this and bet that it would likely also degenerate into mess of special cases. But maybe less ugly mess? But I suspect that it could be actually a better strategy. |
people who told me that this way to maintain it (by parsing AST of StreetComplete code) was not going to work were right in taginfo/taginfo-projects#195 I requested removal of current taginfo listing as it is not maintained and outdated overlays were never supported, supporting new quests required adding growing list of exceptions and special handling not sure what is the best way to handle it and is there workable one if someone is interested I can share with them my code and maybe help them a bit (at least with testing) right now I am not planning to work on this as it is not criticial to implement and huge amount of work I will not reopen this issue |
Theoretically you may reverse the behaviour: Let the StreetComplete code "push" it's modified tags to some service. Less abstract my suggestion would be to do the following:
The advantages would be:
If you ned help, I could help with some GitHub Actions thing, though that is likely the easier part of it. 😅 |
That would be effort-prohibitive and would noticeably complicate code for relatively small gain. |
Really? Could not you just have a CLI calling into the quest functions and extracting the required stuff as said… |
@rugk For modified tags, you could also have an external service that just looks at minutely diffs (or occasionally parse Full history dump) to see all changes that StreetComplete produced, and build database of However, Taginfo projects are supposed to show all tags the project uses, and not only those that they modify (i.e. it should include tags that are used in read-only ways too). E.g. |
This is also not what I meant. I meant a CLI Kotlin tool (wrapped in Gradle or whatever, also whatever language actually, Kotlin just makes sense) just like I also found the PR for the other mentioned example: #2523 |
Ah sorry should read til the end…
Yeah okay, but would not "modify" be a good first step? I guess r/o access is not that often in there, and IIRC one can also differentiate in taginfo whether a tag is added or how it is used (at least I see an auto-generated description)? So we would not include wrong data, just maybe not complete one… until such cases of "r/o accessed-tags" are also implemented? |
Perhaps; AFAICT that it is how it was generated in the past, so one may argue that something is better than nothing, especially as I can put a disclaimer there. Than again, it might be argued that the partial data (especially one skewed far to one side) paints the wrong picture about usage of the tags, so that it is more harmful then useful. 🤷♂️
Well, I wouldn't even dare to guess. There is certainly much more reading of tags then writing of them. And even the few ways I've seen seem complex enough to try to handle, and I'm sure there are plenty more I don't even know about, so I feel it might be some work to even make an attempt at educated guess 🤔
@matkoniecz if you can link the code, it would be interesting to see how complex / kludgy it is currently, and how more might need to be done for it to be more useful... |
As a starting point, what about listing out the tags and keys that StreetComplete quests use to identify candidate questions? The filter DSL seems to be much more formulaic, possibly amenable to a scraping technique. A mapper consulting taginfo would still find it helpful to know that StreetComplete cares about a given key, even if they don’t know whether it writes out that key. |
https://github.com/matkoniecz/Zazolc/blob/taginfo/buildSrc/src/main/java/UpdateTaginfoListingTask.kt copy in case I will delete this branch: https://gist.github.com/matkoniecz/03c812162ba43654188c2ad537e4956a |
Would not a kinda easy refactoring be that, if tags are used not in that SC overpass-like DSL, all quests should somehow register such tags as constants/list or in a function or so (or whatever data structure), where they query it. This function can then be accessed from another function which collects all these tags. Again, I would advise against scraping and adjust the SC quests source code to make it easier to implement. |
By the way, if anyone is going to look into this again, I recommend NOT including the tags in the quest filters in the list of "tags used by StreetComplete" because these tags are just used for filtering (to e.g. find ambiguous data) and may also include deprecated tags etc. It would be quite misleading to display them as "used by StreetComplete". Rather, a script that runs through the codebase should rather look for tags that are actually tagged by StreetComplete, i.e. added or modified. Even tags deleted by StreetComplete should not be included, as these, again, might be deprecated tags that are just cleaned up. |
It is deployed to taginfo as it is superior to what was deployed already - see https://taginfo.openstreetmap.org/projects/streetcomplete#tags and for example https://taginfo.openstreetmap.org/keys/addr:place#projects
Overall I will close this issue, as I have no idea how to provide non-nightmarish solution that can implement it. I am planning to support what I created, so feel free to ping me here or create an issue at https://github.com/matkoniecz/Zazolc/issues if you spot anything wrong.
See https://github.com/matkoniecz/Zazolc/tree/taginfo and more specifically https://github.com/matkoniecz/Zazolc/blob/taginfo/buildSrc/src/main/java/UpdateTaginfoListingTask.kt and https://github.com/matkoniecz/Zazolc/blob/taginfo/res/documentation/taginfo_listing_of_tags_added_or_edited_by_StreetComplete.json
original issue:
I actually have an idea how it can be done. Creating an issue to prevent (really unlikely) conflict with someone else also working on it.
The previous attempt was #2754 and there was also an earlier attempt at manual handling.
@westnordost
replaceShop
- do you think that taginfo listing should also list all that tags that may be added on "shop has changed" and on specifying more detail onshop=yes
?(My opinion is that yes, it should be listed - but as it requires extra work I prefer to ask before even designing this)
@westnordost
Would you be OK with partially manual/hardcoded answers and handling for some tricky cases and quests? I think it is not avoidable without spending outrageous effort on this.
The text was updated successfully, but these errors were encountered: