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 Postgres POSIX regex operators close #4317 #6172

Closed

Conversation

christophediprima
Copy link
Contributor

@christophediprima christophediprima commented Nov 11, 2020

Description

Add support for following Postgres operators:

  • _regex: ~
  • _iregex: ~*
  • _nregex: !~
  • _niregex: !~*

Changelog

  • CHANGELOG.md is updated with user-facing content relevant to this PR.

Affected components

  • Server
  • Console
  • CLI
  • Docs
  • Community Content
  • Tests

Related Issues

#4317

Solution and Design

Steps to test and verify

Limitations, known bugs & workarounds

Server checklist

Catalog upgrade

Does this PR change Hasura Catalog version?

  • No

Metadata

Does this PR add a new Metadata feature?

  • No

GraphQL

  • New GraphQL schema is being generated:
    • New fields in String_comparison_exp

Breaking changes

  • No Breaking changes

@hasura-bot
Copy link
Contributor

Beep boop! 🤖

Hey @christophediprima, thanks for your PR!

One of my human friends will review this PR and get back to you as soon as possible.

Stay awesome! 😎

@CLAassistant
Copy link

CLAassistant commented Nov 11, 2020

CLA assistant check
All committers have signed the CLA.

@netlify
Copy link

netlify bot commented Nov 11, 2020

Deploy preview for hasura-docs ready!

Built with commit 327abfc

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

@christophediprima
Copy link
Contributor Author

This is my first pull request. It is not fully completed yet. Any comment or help to complete it is welcome :)

@abooij
Copy link
Contributor

abooij commented Nov 11, 2020

@christophediprima You can indicate that the PR is not yet ready for review by putting it in a "draft" state. You can do it before you open it, or, for this PR which is already open, click "Convert to draft" on the right-hand side under the "Reviewers" section.

@christophediprima christophediprima marked this pull request as draft November 11, 2020 12:20
@christophediprima christophediprima force-pushed the 4317-posix-operators branch 2 times, most recently from ee1c66c to d1f0ec8 Compare November 12, 2020 17:18
@christophediprima christophediprima marked this pull request as ready for review November 12, 2020 17:21
@christophediprima christophediprima requested a review from a team as a code owner November 12, 2020 17:21
@christophediprima christophediprima force-pushed the 4317-posix-operators branch 4 times, most recently from ea37fd2 to 6e3b6cc Compare November 13, 2020 09:04
@christophediprima
Copy link
Contributor Author

christophediprima commented Nov 13, 2020

@abooij I think I am done with the code. Could you help me with the PR remaining checks? I am not sure if they are yes or no.

@christophediprima
Copy link
Contributor Author

@abooij how long do you think it could take to be integrated into a new version?

@abooij
Copy link
Contributor

abooij commented Nov 21, 2020

Hey @christophediprima! Sorry, I missed your message from last week. Thanks for reminding me, I'll have a look at this PR on Monday.

Copy link
Contributor

@abooij abooij left a comment

Choose a reason for hiding this comment

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

Thanks for this PR, @christophediprima. The Haskell code has been developed very nicely, following the pattern of the other operators accurately. I have no substantial comments on the code itself. Please add one comment for future reference (since this work is overlapping some other work that's currently taking place, and needs to be coordinated somewhat). And I've attempted to improve some test descriptions somewhat.

@beerose, who could review the console part of this PR?

@marionschleifer, could you review the docs for this?

@tirumaraiselvan, could you please review the changelog?

@abooij abooij added c/console Related to console c/docs Related to docs c/server Related to server community-contrib 🙏🏽 labels Nov 23, 2020
@beerose beerose self-requested a review November 23, 2020 12:03
Copy link
Contributor

@beerose beerose left a comment

Choose a reason for hiding this comment

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

Thanks for the PR!
Approving console changes.

Copy link
Contributor

@marionschleifer marionschleifer left a comment

Choose a reason for hiding this comment

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

Docs 👍

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.

Thanks for the wonderful PR but I think we should use regex as the base operator name and also use the existing conventions for "not" and "case insensitivity" for the derived operators: https://github.com/hasura/graphql-engine/pull/6172/files#r530185924

CHANGELOG.md Outdated Show resolved Hide resolved
@abooij
Copy link
Contributor

abooij commented Nov 25, 2020

@christophediprima

Should I keep resolving conflicts on CHANGELOG.md @abooij? Or will @tirumaraiselvan take care of this? sweat_smile

As a rule of thumb, during the review phase of a PR, it's not necessary to resolve every conflict, especially e.g. in the changelog. If there are any substantial conflicts, e.g. another PR has been merged that touches a lot of the same code, then it would be wise to resolve the conflict even before your PR is accepted, so that your PR doesn't go out of date too much.

When you do resolve conflicts, in this repository we prefer that you do so using merge commits rather than rebases and force-pushes, since:

  1. we end up squashing the entire PR anyway
  2. this way we can see how you resolved certain conflicts, and how you are responding to review comments, etc.

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.

@christophediprima Thank you. Just the final minor thing: can you revert the changes to cli-ext/package.json and cli-ext/tests/sdl/payload.js files so they are out of this changeset. I think it's just a lint change.

@christophediprima
Copy link
Contributor Author

@tirumaraiselvan cli-ext/tests/sdl/payload.jshas some changes. I fixed the linting issue.

@tirumaraiselvan tirumaraiselvan changed the title Support Postgres POSIX operators close #4317 Support Postgres POSIX regex operators close #4317 Nov 26, 2020
@christophediprima
Copy link
Contributor Author

@tirumaraiselvan do I still have to do something or is everything ok? :)

@abooij
Copy link
Contributor

abooij commented Nov 26, 2020

@christophediprima Congrats, all lights are green! We'll take this up and let you know if there's anything else you can do.

@hasura-bot
Copy link
Contributor

Thanks for your contribution! Your changes have been merged successfully.

hasura-bot added a commit that referenced this pull request Nov 27, 2020
Co-authored-by: christophediprima <dipdipdip84@gmail.com>
Co-authored-by: dip <dipdipdip84@gmail.com>
Co-authored-by: Auke Booij <auke@hasura.io>
Co-authored-by: Antoine Leblanc <antoine@hasura.io>
GITHUB_PR_NUMBER: 6172
GITHUB_PR_URL: #6172
GitOrigin-RevId: 5192d23
@hasura-bot hasura-bot closed this Nov 27, 2020
@abooij
Copy link
Contributor

abooij commented Nov 27, 2020

@christophediprima: @hasura-bot seems to have been confused momentarily about your PR, but rest assured it was merged successfully!

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
c/console Related to console c/docs Related to docs c/server Related to server community-contrib 🙏🏽 merged
Projects
None yet
Development

Successfully merging this pull request may close these issues.

7 participants