@pablobm commented on this pull request.
Although there's a change of behaviour (enriching the `no_comment`), I'm ok
with it. It simplifies the code and I think makes sense. Looking through the
current translated strings, I don't see anything that would misfire there.
If anything, I'd separate the refactor and the change of behaviour into
separate commits, and add a test for `Changeset#comment` (and `#comment?`).
> @@ -25,7 +25,7 @@ atom_feed(:language => I18n.locale, :schema_date => 2009,
:type => "application/osmChange+xml"
if !changeset.tags.empty? && changeset.tags.key?("comment")
I think an additional API change would be good here, to completely abstract the
comment living in a tag:
```suggestion
if changeset.comment?
```
What I'm not sure is why the empty check on `tags` was necessary :thinking:
> @@ -200,6 +200,8 @@ def save_with_tags!
end
end
+ def comment = tags["comment"].to_s.presence
Is the `.to_s` required here? Just wondering, I'm not sure. If the value is
nil, `#presence` will have the same behaviour, unless I'm much mistaken.
I appreciate this is just copied from the existing code, but since we are
improving this, we might as well consider this question.
--
Reply to this email directly or view it on GitHub:
https://github.com/openstreetmap/openstreetmap-website/pull/6597#pullrequestreview-3561782805
You are receiving this because you are subscribed to this thread.
Message ID:
<openstreetmap/openstreetmap-website/pull/6597/review/[email protected]>_______________________________________________
rails-dev mailing list
[email protected]
https://lists.openstreetmap.org/listinfo/rails-dev
