Skip to content
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

Refactor trace api_update to avoid duplicating tags #1614

Merged
merged 3 commits into from Aug 30, 2017

Conversation

gravitystorm
Copy link
Collaborator

This PR refactors the trace api_update method to work on existing trace objects, rather than creating temporary 'shadow' trace objects. In doing so it refactors the from_xml and from_xml_node model methods to be instance methods instead of class methods.

Advantages:

Concerns:

  • Doesn't avoid deleting and recreating tag objects when the tags are the same (but neither does the tagstring= method, afaict)
  • Other classes (Node, Changeset etc) all use the class method approach for from_xml, rather than instance methods. I don't know whether they should be refactored too, or whether there's some advantage of the class method approach that I haven't understood.

Setting the new tags with the = operator takes care of removing the
old ones, and is the same approach as taken by the tagstring= method.

Fixes openstreetmap#1600
@tomhughes
Copy link
Member

tomhughes commented Aug 18, 2017

Assigning to tags is enough to trigger deletion of the old tags I take it?

There is of course a fundamental difference between traces and other objects in that we don't keep multiple versions of traces - so it makes sense to have from_xml on a node be a constructor that creates the new object we are going to save that replaces the current version. Though I guess we don't keep history for changesets either.

I certainly think that if we go this route we should change the method name to update_from_xml or something so that we don't have different models using the same name in different ways.

@gravitystorm
Copy link
Collaborator Author

Assigning to tags is enough to trigger deletion of the old tags I take it?

Yes. From http://api.rubyonrails.org/classes/ActiveRecord/Associations/ClassMethods.html#method-i-has_many :

"collection=objects : Replaces the collections content by deleting and adding objects as appropriate."

I certainly think that if we go this route we should change the method name to update_from_xml or something so that we don't have different models using the same name in different ways.

OK, I can do that.

They behave differently from the other from_xml methods on other models.
Copy link
Member

@tomhughes tomhughes left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Looks good to me now.

@tomhughes tomhughes merged commit b4be559 into openstreetmap:master Aug 30, 2017
@gravitystorm gravitystorm deleted the trace_tags branch April 17, 2018 08:47
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

PUT on gpx_file in API does not remove old tags
2 participants