Add warning for features without start date #193
Conversation
data/core.yaml
Outdated
| title: Empty start date | ||
| feature: | ||
| message: "Start date shouldn't be empty" | ||
| reference: "Features should have a start date" |
There was a problem hiding this comment.
A reference should explain why the user should care about the validation warning. It appears below the warning as help text. This wiki documentation could be a good starting point, but it needs to be more concise here.
We could also use this opportunity to inform the user of the EDTF field, in case they don’t know the precise date. Mappers sometimes feel pressured to resolve warnings that pop up, but we would rather have start_date:edtf express a guesstimate than have an arbitrary value in start_date just to silence the warning. If you can think of a way to hook up a dynamic fix to open that field, that would be swell, but just mentioning it here would get the job done too.
There was a problem hiding this comment.
I changed the text to explain the reason better. I also tried to add a dynamic fix to focus on the EDTF date field, but i couldn't figure out how. It can be done using document.getElementById(), but I believe that doesn't fit the project's style properly. There should be an easy way to do it using context.ui(), I will keep trying for a while
|
Unfortunately, there’s a minor merge conflict in tags.js because #194 synchronized this fork with the upstream repository. The changes in openstreetmap/iD@9779f32 and openstreetmap#9424 are unrelated to the changes you’re making, so hopefully it’s a straightforward conflict to resolve, but let me know if you’d like more guidance on how to resolve it. |
pedromml
force-pushed
the
start_date_validation
branch
from
46544bc
to
1a0f759
Compare
|
#195 should address the unrelated test failures you’ve been running into. If you need to get around the failures in the meantime, you can temporarily cherry-pick the commits or modify this path to include only the specs you want to run. However, I recommend testing the full suite anyways, to avoid making the same mistakes I made. |
There was a problem hiding this comment.
Thanks for iterating on this PR. It’s in great shape now. Since we’ve been getting by without this warning for so long, there will be a lot of issues to fix. Some mappers may feel overwhelmed and ask us to make the rule more lenient, but I think this will be good motivation for the community to clean up the database. If it really becomes a problem, we can always tweak the rule in a separate PR.
| }); | ||
|
|
||
| it('ignores way with start_date:edtf tag', function() { | ||
| createWay({ 'start_date:edtf': '1950' }); |
There was a problem hiding this comment.
Eventually, we may decide to warn about a missing start_date in this case as well, since the map renderer doesn’t know anything about start_date:edtf yet. Typically, we’ve been considering start_date:edtf to be a complement to start_date rather than a replacement for it. However, I think what you have currently is correct. I also made the same assumption in #188: the suggested fix for that warning only sets start_date:edtf but not start_date.
This PR adds the
empty_start_datevalidation, that shows a warning on ID if a feature does not havestart_dateorstart_date:edtftags.Features that have no tags do not get this warning, as they already have another.
Features that have the
natural,waterwayorwatertags also don't get this warning, as these features usually lack a rigorous start date.