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.

Sign up for GitHub

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

Jump to bottom

Improve base.rb & update active_hash #254

Merged
merged 8 commits into from
Aug 10, 2021
Merged

Improve base.rb & update active_hash #254

merged 8 commits into from
Aug 10, 2021

Conversation

senhalil
Copy link
Contributor

@senhalil senhalil commented Jul 8, 2021 •
edited
Loading

For has_many and belongs_to

Closes optimizer-api/-/issues/446

@senhalil senhalil requested a review from braktar July 8, 2021 17:05
braktar
braktar reviewed Jul 9, 2021
View reviewed changes
Copy link
Contributor

@braktar braktar left a comment

Choose a reason for hiding this comment

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

Regarding this test error : https://github.com/Mapotempo/optimizer-api/pull/254/checks?check_run_id=3021525858#step:5:1659
It seems like the getter for end_point_id may not work as expected

@senhalil
Copy link
Contributor Author

senhalil commented Jul 9, 2021

It turns out this is due to a weird behaviour in active_hash. It is possible to create two Point objects as {id: 1} and {id: "1"} but it is not possible to to get {id: "1"} because there is a to_s in the insert and find methods.

This means if the user defines two points -- one with integer, the other with string -- there is no warning (which is cool) but then start_point_id: "1" and start_point_id: 1 will both point to the Point with id: 1. This is just calling for trouble because "1" and 1 are not the same thing and to_s is not necessary because Hash can handle integer keys. I will open a PR in active_hash repo

@senhalil senhalil force-pushed the dev branch 2 times, most recently from cbbe508 to 3b55495 Compare July 12, 2021 15:01
@senhalil
Copy link
Contributor Author

senhalil commented Jul 12, 2021 •
edited by braktar
Loading

@senhalil senhalil force-pushed the dev branch 4 times, most recently from cdbcc3b to 921388c Compare July 15, 2021 08:50
@senhalil senhalil changed the title Improve getter/setter of base.rb Improve getter/setter of base.rb & update active_hash Jul 15, 2021
@senhalil senhalil changed the title Improve getter/setter of base.rb & update active_hash Improve base.rb & update active_hash Jul 15, 2021
@braktar braktar requested a review from fonsecadeline July 19, 2021 10:00
@braktar braktar mentioned this pull request Jul 19, 2021
Closed
5 tasks
@senhalil senhalil force-pushed the dev branch 7 times, most recently from 323592f to d49f2ca Compare July 26, 2021 09:20
@senhalil senhalil force-pushed the dev branch 2 times, most recently from 8c8a2ec to 783cd9a Compare July 30, 2021 15:22
@senhalil
Improve base.rb
cd94b1d 
Remove dead code
Fix y_ids= operators
Find raises if id does not exists
Add getter for has_many ids
Fix id getter of belongs_to
Functions keep the id(s) up-to-date
Generate only necessary id functions
Generate only correct ids functions
@senhalil senhalil mentioned this pull request Aug 2, 2021
Merged
1 task
fab-girard
fab-girard reviewed Aug 10, 2021
View reviewed changes
lib/tsp_helper.rb Outdated Show resolved Hide resolved
@fab-girard fab-girard merged commit 721e32b into Mapotempo:dev Aug 10, 2021
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Reviewers

@fonsecadeline fonsecadeline fonsecadeline approved these changes

@braktar braktar braktar approved these changes

@fab-girard fab-girard fab-girard approved these changes

Assignees
No one assigned
Labels
None yet
Projects
None yet
Milestone
No milestone
Development

Successfully merging this pull request may close these issues.
4 participants
@senhalil @fab-girard @braktar @fonsecadeline