Fixing missing amenity name labels regression

[kocio-pl]

Resolves #1800.

Fixing YAML/MML broken in commit 4745130. It is still spaghetti code, which is hard to compare (long lines and all the layers together), so please check if I didn't make a mistake!

I have checked it works with:

• school
• kindergarten
• university
• college
• grave_yard

so all the missing labels are back.

[matkoniecz]

Why in some cases order of listed amenities was changed from 'pub', 'restaurant' to 'shelter', 'atm' (coalesce in amenity-points-poly) and in some reverse happened (where clause in text-poly).

I am not sure whatever one order is preferable over another but it would be better to make it consistent.

[HolgerJeromin]

I propose a PR with a commit fixing the problem and a second commit adding new lines and sorting stuff.
With this strategy review if easier and we are faster on at the desired yaml style.

[kocio-pl]

I was trying to make as few changes as possible and just reverted the broken lines (adding only community_centre when needed). This is meant as a quick fix (like v2.34.1), I don't think more changes are sane before we will introduce the solution @gravitystorm has proposed lately.

[kocio-pl]

Does anybody from the team plan to merge this code or is there any problem with it which needs to be solved before?

I would really like to have it done ASAP and then go further with code cleaning to avoid such problems in the future.

[HolgerJeromin]

You reverted the commit and readded the needed value.
That is probably a good idea, but to review we have to compare to the state before the broken commit.
Another approach would be fixing the broken things from the current state, so the github diff is showing exactly what you have fixed.

But probably the maintainer are skilled enough to be able to review both approaches. :-)

[matthijsmelissen]

As far as I'm concerned, I haven't had time to review any pull requests lately. I will look at it as soon as possible!

In the future, I'll try to be more explicit when I'm not merging something because I'm not convinced enough about the merits of a PR (but that was not the case here).

@HolgerJeromin Either way is fine with me, that's not the reason nothing has happened so far.

[kocio-pl]

As far as I'm concerned, I haven't had time to review any pull requests lately. I will look at it as soon as possible!

Thanks!

But I'm confused: are you assigned for this task and the rest of the team in general does not pull any code in - or it works in some other way?

In the future, I'll try to be more explicit when I'm not merging something because I'm not convinced enough about the merits of a PR (but that was not the case here).

It's always good to know what is the status of a PR not being actively discussed/reworked. Few days with no additional input should not be a problem (if it's not something serious like this regression), but a week or two would be a good mark of PR being forgotten or having some general problems that should be resolved (or just to be closed for some reason).

@matkoniecz wrote elsewhere about the experience of being uncertain with creating PRs and this is also the problem I share sometimes in this repo.

[matthijsmelissen]

But I'm confused: are you assigned for this task and the rest of the team in general does not pull any code in - or it works in some other way?

No, any of the maintainers can accept pull request, there is no agreed distribution of tasks within the team.

but a week or two would be a good mark of PR being forgotten or having some general problems that should be resolved (or just to be closed for some reason).

It happens regularly that there are two weeks in a row in which I have no time to spend on this project. Just a natural thing for a project run by volunteers, and no indication of the quality of proposed PR's!

[matkoniecz]

But I'm confused: are you assigned for this task and the rest of the team in general does not pull any code in - or it works in some other way?

At least for me it is 100% hobby so whenever I am in the mood to review PRs and I have spare time I am doing this. Otherwise I am doing other things.

And yes, any maintainer may accept any pull requests (except his own) and typically there are always PRs waiting for review.

It's always good to know what is the status of a PR not being actively discussed/reworked.

Whenever I see some kind of problem I comment immediately (here it was #1825 (comment)), in case of starting review that will last for a long time I assign PR to myself (like at #991 (comment) ). Anything else means that I did nothing with given PR.

Few days with no additional input should not be a problem (if it's not something serious like this regression), but a week or two would be a good mark of PR being forgotten or having some general problems that should be resolved (or just to be closed for some reason).

It means that PR waits for somebody who will review it. It is not possible that PR will be lost - we have no huge backlog of proposed patches and github interface is OK.

And on finding "general problems that should be resolved" people will give comment.

[kocio-pl]

Thank you both for the comments!

Sorry for sounding too alarming probably - I was just not sure how is it supposed to be working (especially in the context of this fix). Lack of time for OSM is something I can perfectly understand because I'm doing it also as a pure hobbyist, it was just not clear to me if it may be also some other problem with decision making or workflow within a project.

[kocio-pl]

@gravitystorm Any chance to release v2.35 containing this fix anytime soon (there were more changes lately, so naming it v2.34.1 wouldn't make sense probably)?

[matkoniecz]

@kocio-pl You may want to subscribe to updates from https://github.com/openstreetmap/chef/ (openstreetmap/chef#36 is relevant in that case).

[kocio-pl]

Thanks!