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

Support JSON extract path expression #20

Closed
wants to merge 5 commits into from
Closed

Conversation

heywhy
Copy link
Contributor

@heywhy heywhy commented Jun 20, 2022

Previously, queries that check against JSON columns couldn't be used alongside this adapter. With the changes in this PR, we can now do this. see an example below:

from(e in Model.Employee)
|> where([e], e.metadata["twitter"] == "@andrew_fuller")
|> Repo.one!()

Also, a function called flush_tables was added to the adapter module to allow us to drop all data in a repository considering some tests might pollute the database and we have no way to implement postgres-like partitioning at the moment.

@evadne
Copy link
Owner

evadne commented Jun 27, 2022

@heywhy Thank you very much, I am reviewing this now.

@evadne evadne self-requested a review June 27, 2022 19:38
Copy link
Owner

@evadne evadne left a comment

Choose a reason for hiding this comment

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

My initial view: Very pleasant set of changes, good job and thank you for the contribution. Please can you review my comments and advise. Thank you again for your time.

lib/etso/adapter.ex Outdated Show resolved Hide resolved
lib/etso/adapter.ex Outdated Show resolved Hide resolved
test/northwind/repo_test.exs Outdated Show resolved Hide resolved
test/support/northwind/model/employee.ex Show resolved Hide resolved
lib/etso/ets/match_specification.ex Outdated Show resolved Hide resolved
lib/etso/adapter/types/map.ex Outdated Show resolved Hide resolved
test/northwind/repo_test.exs Outdated Show resolved Hide resolved
@evadne
Copy link
Owner

evadne commented Jun 28, 2022

@heywhy FYI, reconstructing branch as your changes were from 0.1.6 and included undesired cross merge from master 1.0.1 so history needs to be refactored.

@evadne
Copy link
Owner

evadne commented Jun 28, 2022

Reconstitution of WIP @ feature/gh-20-json-extract-path

@evadne
Copy link
Owner

evadne commented Jun 28, 2022

@heywhy Please retest branch feature/gh-20-json-extract-path @ 31f2c3c, then pull this to your fork (fast forward only, do not create merge commits), then revise the working branch in this PR to allow correct attribution via GitHub history.

Please note active_tables is removed, you should do the following instead,

for model <- [ModelA, ModelB] do
  Repo.delete_all(model)
end

As I have implemented delete_all support.

@evadne evadne self-requested a review June 28, 2022 21:15
@evadne
Copy link
Owner

evadne commented Jun 29, 2022

@heywhy do not push to develop, use the same branch I published and revise this PR to use that branch to conform to merge requirements.

@heywhy
Copy link
Contributor Author

heywhy commented Jun 29, 2022

@heywhy do not push to develop, use the same branch I published and revise this PR to use that branch to conform to merge requirements.

That's a mistake on my end. I thought you wanted the commits squashed or something. I will close this PR and create a new PR from the other branch as everything from the other branch works as expected.

@heywhy heywhy closed this Jun 29, 2022
@evadne
Copy link
Owner

evadne commented Jun 29, 2022

Yes that would be good though name the branch with feature/gh-21 or so. @heywhy

@heywhy
Copy link
Contributor Author

heywhy commented Jun 29, 2022

Also, can you have a part in the readme that tells contributors what merge strategy should be used so as to avoid a situation like mine?

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.

2 participants