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

Auto remove line breaks on Columns.Raw #544

Merged

Conversation

iruizmar
Copy link
Contributor

@iruizmar iruizmar commented Apr 9, 2024

What kind of change does this PR introduce?

Bug fix

What is the current behavior?

Solves #543

What is the new behavior?

Columns.Raw now automatically gets rid of line breaks

Additional context

Let me know if that's not the correct place to introduce it.
Should we also auto add trimIndent()?

@jan-tennert
Copy link
Collaborator

jan-tennert commented Apr 9, 2024

Hey, thanks for this PR! After looking at the other client libs, it seems like what they're doing something different:
https://github.com/supabase/postgrest-js/blob/7c3dd21fb962c35e88b88cc7a789fcfcb692af42/src/PostgrestQueryBuilder.ts#L72-L84
This removes newlines but also any whitespaces while allowing whitespaces when quoted.
We should probably also do it like this, maybe define a extension function at the bottom of Columns.kt and clean the parameter when creating the column instance:

private fun String.clean(): String {
    var quoted = false
    val regex = Regex("\\s")
    return this.map {
        if (it == '"') {
            quoted = !quoted
        }
        if (regex.matches(it.toString()) && !quoted) {
            ""
        } else {
            it
        }
    }.joinToString("")
}

@grdsdev
Copy link

grdsdev commented Apr 9, 2024

Hi all,

@jan-tennert suggestion is the way to go, to keep same behavior across client libraries.

@iruizmar
Copy link
Contributor Author

iruizmar commented Apr 10, 2024

That's cool, thanks for checking other client libraries to see their behaviour.
I applied the change.
Although, I'm thinking that this is a hidden feature since it's not added to the docs. I can't see it on the JS client library either.

@grdsdev
Copy link

grdsdev commented Apr 10, 2024

@iruizmar what do you mean by hidden feature? I see it more as a convenience for using multiline select queries.

You wish we had some doc entry mentioning that it is possible to use multiline queries?

Copy link
Collaborator

@jan-tennert jan-tennert left a comment

Choose a reason for hiding this comment

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

We should probably add unit tests for multiline columns and quoted whitespaces, but that can be in another PR. Thanks!

@jan-tennert jan-tennert merged commit a017797 into supabase-community:master Apr 10, 2024
8 checks passed
@iruizmar
Copy link
Contributor Author

@iruizmar what do you mean by hidden feature? I see it more as a convenience for using multiline select queries.

You wish we had some doc entry mentioning that it is possible to use multiline queries?

We're now allowing spaces only if they are quoted. I think users won't know this feature if it's not documented. Even if I, honestly, can't see a usecase in which you'd want to have a white space.

So yes, adding a section in the docs about this feels the correct approach.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Projects
None yet
Development

Successfully merging this pull request may close these issues.

None yet

3 participants