这是indexloc提供的服务，不要输入任何密码
Skip to content

clean SQL text functions #6017

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
Merged
merged 6 commits into from
Oct 22, 2020
Merged

clean SQL text functions #6017

merged 6 commits into from
Oct 22, 2020

Conversation

@nicuveo
Copy link
Contributor

@nicuveo nicuveo commented Oct 14, 2020 •
edited
Loading

Description

This PR cleans text manipulation functions out of the SQL directory, and merges them with the ones defined in Data.Text.Extended. It deletes the DQuote typeclass, in favour of the already redundant ToTxt. It also cleans some existing code, by making use of already exising functions such as dquoteList, and by introducing commaSeparated.

@nicuveo nicuveo requested a review from 0x777 October 14, 2020 17:32
@nicuveo nicuveo mentioned this pull request Oct 16, 2020
Merged
@abooij abooij added the c/server Related to server label Oct 16, 2020
@nicuveo nicuveo mentioned this pull request Oct 21, 2020
Merged
@0x777
Copy link
Member

0x777 commented Oct 21, 2020

@nicuveo Why not move those functions to Data.Text.Extended instead of SQL.Text? Are they in any way tied to SQL?

@nicuveo
Copy link
Contributor Author

nicuveo commented Oct 21, 2020

They conflict with Data.Text.Extended: same names, different types... They are used exclusively for SQL generation, hence their presence in SQL. I would definitely be in favour of replacing the ones in Data.Text.Extended by those ones and get rid of SQL.Text entirely. WYT?

@0x777
Copy link
Member

0x777 commented Oct 21, 2020

They conflict with Data.Text.Extended: same names, different types... They are used exclusively for SQL generation, hence their presence in SQL. I would definitely be in favour of replacing the ones in Data.Text.Extended by those ones and get rid of SQL.Text entirely. WYT?

Get rid of SQL.Text!

@nicuveo
Copy link
Contributor Author

nicuveo commented Oct 21, 2020

The only problem is the instances for GraphQL types; they don't really belong in Data.Extended... I could keep them as orphan instances in SQL.Types, but... that feels wrong. :/

@0x777
Copy link
Member

0x777 commented Oct 21, 2020

they don't really belong in Data.Extended

Why not? Since Data.Text.Extended is introducing a typeclass ToTxt, it seems reasonable for the instances to go in that module?

@nicuveo
Copy link
Contributor Author

nicuveo commented Oct 21, 2020

Ah ok! My understanding was that stuff in Data was to belong as if it could be a standalone library. But if not, yeah, of course, that works, will do!

@0x777
Copy link
Member

0x777 commented Oct 21, 2020

We can even use TextShow from this package and avoid defining our own ToTxt typeclass? That way the instances for graphql types for TextShow type will go into graphql-parser library. I guess it depends on what the semantics of ToTxt typeclass are?

@nicuveo
Copy link
Contributor Author

nicuveo commented Oct 21, 2020

That seems like a good idea! I'll just need to patch the parser first. But sounds good to me!

@nicuveo nicuveo mentioned this pull request Oct 21, 2020
Merged
@nicuveo
Copy link
Contributor Author

nicuveo commented Oct 21, 2020

Done! It's a bit verbose, but at least that's out of the way now. :)

@netlify
Copy link

netlify bot commented Oct 21, 2020 •
edited
Loading

Deploy preview for hasura-docs ready!

Built with commit b72fa98

https://deploy-preview-6017--hasura-docs.netlify.app

@nicuveo
Copy link
Contributor Author

nicuveo commented Oct 21, 2020

Pushed a few more things: introduced commaSeparated, and replaced several instances of map dquote . toList by the appropriate text function.

@nicuveo
Copy link
Contributor Author

nicuveo commented Oct 22, 2020

Fixed the kerfuffle re: TextShow. This should be better!

tirumaraiselvan
tirumaraiselvan approved these changes Oct 22, 2020
View reviewed changes
Copy link
Contributor

@tirumaraiselvan tirumaraiselvan left a comment

Choose a reason for hiding this comment

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

no cl required

@nicuveo nicuveo merged commit f5da1b3 into hasura:master Oct 22, 2020
@nicuveo nicuveo deleted the clean-text branch October 22, 2020 16:23
@abooij abooij mentioned this pull request Nov 4, 2020
Merged
6 tasks
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Reviewers

@0x777 0x777 0x777 approved these changes

@tirumaraiselvan tirumaraiselvan tirumaraiselvan approved these changes

Assignees

No one assigned

Labels

c/server Related to server no-changelog-required

Projects

None yet

Milestone

No milestone

Development

Successfully merging this pull request may close these issues.

4 participants

@nicuveo @0x777 @tirumaraiselvan @abooij