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

Feat/fixed many2 many and added update model #124

Conversation

carlesnunez
Copy link

@carlesnunez carlesnunez commented Apr 28, 2017

The next PR implements a feature that allow the developer to update instead of create a record if the props.id that we are passing to the create are already as a record on our store. It also, fixes a many2many error.

@tommikaikkonen
Copy link
Collaborator

Hey @carlesnunez,

Thanks for submitting a PR! I noticed your other PR #112, but I'll leave comments here

  • Looks like your second commit changes a lot of indentation from 4 to 2 spaces, and there's also some reordering of functions in Model. It's making it hard to see what's changed in logic. Can you revert that commit and only apply the logic changes you're proposing? The project has an eslint config, which you can use with command make lint to check that code adheres to the project style.
  • Can you provide a failing test case for the fix in 1be587a?
  • Updating an instance if one already exists, and creating one if it doesn't, is a common pattern, but I think it could be problematic if it's implicit in the .create method. If I don't read the docs, I wouldn't expect create to update the instance if it already exists. And if it does, I might want to know if the instance actually existed already or not. E.g. in Django you can use Model.objects.update_or_create that takes 1) lookup props and 2) default props. I'm sure other ORM's have a similar method too. I think it could make more sense to put this functionality in it's own method, such as Model.updateOrCreate().

@plandem
Copy link
Collaborator

plandem commented May 2, 2017

I would name it upsert (update or insert) like the term that is using for mySQL, Postgres and etc

@plandem
Copy link
Collaborator

plandem commented May 15, 2017

added upsert here

7fdec84

@plandem
Copy link
Collaborator

plandem commented May 15, 2017

will duplicate my answer here:

  1. PR that you mentioned has multi issues

  2. there was a request to revert formatting to review changes( that I'm totally agree) for a quite long time without any response/feedback - I just don't have idea how long we must wait till reject.

  3. my commit mainly patches "system" files and add the "upsert" feature that had some positive feedback about naming at original issue and has very simple implementation that already mentioned few times at others issues/comments. So I don't see any problem here.

P.S. It's all about contributing to library and improving it, but not about personal ego and copyrighting every line of code. As for me, I would be fine with such merging.

@plandem
Copy link
Collaborator

plandem commented May 15, 2017

maybe it's a good idea to create authors.md or contributors.md file with list of people that have some activity at project (issues/PR).

@carlesnunez
Copy link
Author

I am agree with that @plandem I close this PR then!

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.

None yet

3 participants