Add support new scheme tagging #83
Conversation
|
Thanks a lot, this looks great. Will test it out later this week. |
zlant
force-pushed
the
feature/conditional-tags
branch
3 times, most recently
from
164effb to
5adcfed
Compare
|
Testing this out locally - the It correctly displays if I toggle the I can't tell if it's a new or old bug, I assume it's caused by https://github.com/zlant/parking-lanes/pull/83/files#diff-3f0503e42c14ce29b4c2ed15add2dfb48b79118f309b6f96aed25b88d52e283eR332 Should I create a separate ticket for that? |
It old bug, fot not this PR |
| 'no_parking', | ||
| 'no_standing', | ||
| 'no_stopping', | ||
| 'no', | ||
| ] |
There was a problem hiding this comment.
Should free; residents be added?
In Australia, a common scenario is:
- Free parking
- Time limited for public to a few hours
- Unlimited time for residents (permit holders).
I believe this is covered by the tagging guidelines like so:
parking:condition:right=ticket; residents (This is a paid parking with no other restrictions. Residents may also park here with a permit.)
parking:condition:right:conditional=no_parking @ (Oct 1-Apr 30: Mo 10:00-15:00) (You may not park here on Mondays 10-15 during October-April.)
parking:condition:right:residents=Ku (Residents need permit "Ta".)
(Taken from second example on https://wiki.openstreetmap.org/wiki/Key:parking:condition/complex)
| @@ -89,6 +90,7 @@ const parkingLaneTagTemplates = [ | |||
| 'parking:lane:{side}', | |||
| 'parking:lane:{side}:{type}', | |||
| 'parking:condition:{side}', | |||
| 'parking:condition:{side}:conditional', | |||
There was a problem hiding this comment.
Should this also include maxstay:conditional for when stay duration is limited only during the day for example
Also, should parking:condition:{side}:time_interval or parking:condition:{side}:default' be displayed any more? Or should they still be displayed in case there is legacy data?
There was a problem hiding this comment.
Or should they still be displayed in case there is legacy data?
parking:condition:{side}:time_interval``parking:condition:{side}:default'
yes
There was a problem hiding this comment.
Should this also include
maxstay:conditionalfor when stay duration is limited only during the day for example
Will need to be added in the future
| @@ -0,0 +1,56 @@ | |||
| export function parseConditionalTag(tag: string) { | |||
There was a problem hiding this comment.
This seems like some fairly complex logic which I don't follow without some study. Could we add unit tests to the project and some basic tests for this?
I'd recommend Jest, I'm very happy to create a PR with a basic Jest configuration and example test.
There was a problem hiding this comment.
Added in PR #97. Please merge that then rebase this branch on master so we can add tests for this code.
zlant
force-pushed
the
feature/conditional-tags
branch
4 times, most recently
from
ca0bbb4 to
a9f3b43
Compare
zlant
force-pushed
the
feature/conditional-tags
branch
from
a9f3b43 to
33d1e94
Compare
|
|
||
| conditions.conditionalValues = parseConditionsByNewScheme(side, tags) | ||
| if (conditions.conditionalValues.length > 0) { |
There was a problem hiding this comment.
If I understand this correctly, it's attempting to parse by the new schema, then if no relevent tags are found it parses by the old schema.
Rather than trying the old than new schema, could we write a function to convert all old schema tags to new ones (and unit test this heavily to ensure it is correct), then parse by the new schema?
This would also close #93. I added a proposal for this method here: #93 (comment)
There was a problem hiding this comment.
I think there should be a separate independent method that checks for old tags and suggests migration
There was a problem hiding this comment.
Which is called during the opening of the edit panel
There was a problem hiding this comment.
Why suggest migration rather than do it automatically?
- If the user declines the migration we need to parse by the old schema, and support editing using the old schema
- Why should we encourage users to edit to an old schema?
What if we displayed a little banner or "toast" message if the tags are (automatically) migrated? (and the save button would be highlighted too).
There was a problem hiding this comment.
If it's out of scope in this PR I'm happy to draft a PR for what I'm thinking afterwards :)
There was a problem hiding this comment.
Now many parking lots are mapped according to the old scheme and they will not be migrated manually soon.
If the user wants to make minor edits, rather than deal with the new scheme, he should leave this option.
src/utils/opening-hours.ts
Outdated
| @@ -1,6 +1,9 @@ | |||
| import OpeningHours from 'opening_hours' | |||
|
|
|||
| export function parseOpeningHourse(value: string): OpeningHours | 'even' | 'odd' | null { | |||
| export function parseOpeningHourse(value: string | null): OpeningHours | 'even' | 'odd' | null { | |||
zlant
force-pushed
the
feature/conditional-tags
branch
from
47b9a6b to
d7de1b0
Compare
Start impelementation support conditional parking tags