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

* accept unicode arrows #257

Merged
merged 1 commit into from Mar 9, 2016
Merged

Conversation

lvicentesanchez
Copy link
Contributor

Fixes #241

Problem

An unicode arrow, →, in a partial insert or update will make the parser to fail.

Solution

Modify the parser to accept both a single arrow, ->, or its unicode variant, →.

Notes

Closes #241.

Checklist

  • Unit test all changes
  • Update README.md if applicable
  • Add [WIP] to the pull request title if it's work in progress
  • Squash commits that aren't meaningful changes
  • Run sbt scalariformFormat to make sure that the source files are formatted

@getquill/maintainers

@@ -350,8 +350,10 @@ trait Parsing extends SchemaConfigParsing {
Delete(astParser(query))
}

private val arrow = pq"→|->"
Copy link
Collaborator

Choose a reason for hiding this comment

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

out of curiosity, what is the pq interpolator?

Copy link
Contributor Author

Choose a reason for hiding this comment

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

pattern interpolator, the cleanest way I found to support both arrows without code duplication :)

@fwbrasil
Copy link
Collaborator

fwbrasil commented Mar 9, 2016

👍

Approved with PullApprove

@lvicentesanchez
Copy link
Contributor Author

I don't really understand why the build failed. I ran all the tests before creating the PR :-/

@fwbrasil
Copy link
Collaborator

fwbrasil commented Mar 9, 2016

@lvicentesanchez
Copy link
Contributor Author

yes, you are right. I'm afraid it might be be an issue with scoverage and UTF-8 encoding. I will confirm it tomorrow and submit an issue/PR to scoverage if appropriate.

@jilen
Copy link
Collaborator

jilen commented Mar 9, 2016

@lvicentesanchez Seems related to this

@@ -54,3 +54,4 @@ sbt:
- TRAVIS_PULL_REQUEST
- TRAVIS_BRANCH
- ENCRYPTION_PASSWORD
- SBT_OPTS=-Dfile.encoding=UTF-8 -Xms512m -Xmx1536m -Xss2m -XX:ReservedCodeCacheSize=256m -XX:+TieredCompilation -XX:+CMSClassUnloadingEnabled -XX:+UseConcMarkSweepGC
Copy link
Contributor Author

Choose a reason for hiding this comment

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

This fixed the encoding issue... I have used the default options that sbt-extras is using + encoding.

@lvicentesanchez
Copy link
Contributor Author

@jilen I solved it by forcing UTF-8 as the encoding for sbt. Once they publish a new version with the actual fix, we can upgrade and remove my workaround :)

@fwbrasil
Copy link
Collaborator

fwbrasil commented Mar 9, 2016

👍

Approved with PullApprove

@gustavoamigo
Copy link
Contributor

@lvicentesanchez
Copy link
Contributor Author

@gustavoamigo good catch! I will make the change there too :)

@lvicentesanchez
Copy link
Contributor Author

ok... there is something weird going on with this PR. Last 2 build shown an error that has nothing to do with my changes. I'm trying to build the PR again. If everything goes ok, I will rebase it and force pull a new version.

@lvicentesanchez
Copy link
Contributor Author

Ok... so I re-run the build and no problems. The error was related to finagle-async and serialization; it seems to be a transient serialization issue related to the nr of file descriptors.

Does everybody agree with the introduction of the UnicodeArrowParsing trait?

@fwbrasil
Copy link
Collaborator

fwbrasil commented Mar 9, 2016

👍

Approved with PullApprove

1 similar comment
@lvicentesanchez
Copy link
Contributor Author

👍

Approved with PullApprove

@gustavoamigo
Copy link
Contributor

👍

Approved with PullApprove

lvicentesanchez added a commit that referenced this pull request Mar 9, 2016
@lvicentesanchez lvicentesanchez merged commit d2c026a into zio:master Mar 9, 2016
@cjuexuan
Copy link

good job

@lvicentesanchez lvicentesanchez deleted the issues/241 branch March 10, 2016 14:54
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.

None yet

5 participants