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

allow multiple occurencies of builder statements #50

Merged
merged 2 commits into from
Aug 1, 2023

Conversation

xronos-i-am
Copy link
Contributor

No description provided.

@xronos-i-am
Copy link
Contributor Author

reopen #42 because of loosing code in master branch

@ermolaev
Copy link
Contributor

@SamSaffron

now mini-sql behaves incorrectly

MINI_SQL.build('(/*select*/ from orders) union (/*select*/ from line_items)').select('user_id').query
(SELECT user_id from orders) union (/*select*/ from line_items)

this PR fix it

(SELECT user_id from orders) union (SELECT user_id from line_items)

@SamSaffron
Copy link
Member

I support this, but it is technically a breaking change, going to have to bump versions for it.

@SamSaffron
Copy link
Member

I wonder if we need a new param to explicitly opt for polymorphic.

@ermolaev
Copy link
Contributor

@SamSaffron hello, I come across this problem from time to time in my work

also not work with sql_literal, this PR fix it

builder = MINI_SQL.build(<<~SQL)
  SELECT
    DISTINCT ON (/*snap_grid*/)
    coords
  FROM delivery_offices
  ORDER BY /*snap_grid*/
SQL

builder.sql_literal(snap_grid: 'ST_SnapToGrid(coords, 0.0007, 0.00035)')

can you merge this PR and bump new gem version?

@SamSaffron
Copy link
Member

Yes I am fine with this, but I would like an example added to the README as well here. It feels like a feature we should document.

Also, technically this is a breaking change, we are going to need a full version bump for it, go ahead and do so and also explain in the changelog.

@xronos-i-am
Copy link
Contributor Author

Yes I am fine with this, but I would like an example added to the README as well here

Hello, @SamSaffron. It's done. Do you bump version and add comment to changelog by yourself?

@SamSaffron
Copy link
Member

I am merging it in, will discuss internally if it makes sense to do a full major version bump for this.

Really depends how you squint, this is either a breaking change or bug fix...

@SamSaffron SamSaffron merged commit 6a94f53 into discourse:main Aug 1, 2023
5 checks passed
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Development

Successfully merging this pull request may close these issues.

3 participants