Added decimal highlighting #24

Cxarli · 2015-10-16T13:33:54Z

As discussed here, added decimal highlighting in SQL

winstliu · 2015-10-16T13:44:47Z

Can you add specs to this please?

Cxarli · 2015-10-16T13:49:39Z

@50Wliu How would I add specs? (New to contributing to others' projects)

winstliu · 2015-10-16T15:04:03Z

You can see https://github.com/atom/language-shellscript/blob/master/spec/shell-unix-bash-spec.coffee for an example of specs.

A simple example is also located here: https://github.com/atom/atom/blob/master/CONTRIBUTING.md#specs-styleguide

Cxarli · 2015-10-16T15:16:15Z

That's quite a lot of work for a quick fix... Is

describe 'not', ->
 it 'indicates that the next statement should be turned around', ->
 describe 'turn around', ->
  it 'turns true when false'
  it 'turns false when true'

enough for this piece of code? Where should I place it?

winstliu · 2015-10-16T15:21:43Z

That's quite a lot of work for a quick fix

Yeah, it's just to make sure that your fix doesn't break accidentally in the future.

Anyway, something like this should work:

it 'tokenizes numbers', ->
  {tokens} = 'numbers and stuff here'
  expect(tokens[0]).toEqual value: 'the character', scopes: ['language scopes (Log Cursor Scopes or something in the Command Palette)']
  expect(tokens[1]).toEqual ...etc.

Just place it at the end of https://github.com/atom/language-sql/blob/master/spec/grammar-spec.coffee.

Adds specs

Cxarli · 2015-10-16T15:29:46Z

Added specs, is this good?

winstliu · 2015-10-16T19:27:24Z

grammars/sql.cson

@@ -106,7 +106,7 @@
    'name': 'storage.modifier.sql'
  }
  {
-    'match': '\\b\\d+\\b'
+    'match': '\\b\\d+(\\.\\d+)?\\b'


Is something like 5. valid? If so, this should be changed to \\d*.

So I did a little test:

mysql> INSERT INTO NumberTest (number) VALUES (1.0); mysql> INSERT INTO NumberTest (number) VALUES (1.); mysql> INSERT INTO NumberTest (number) VALUES (.1); mysql> SELECT * FROM NumberTest; +----+--------+ | id | number | +----+--------+ | 1 | 1.0000 | | 2 | 1.0000 | | 3 | 0.1000 | +----+--------+ mysql> SELECT * FROM NumberTest WHERE number=.1; +----+--------+ | id | number | +----+--------+ | 3 | 0.1000 | +----+--------+ mysql> SELECT * FROM NumberTest WHERE number=1.; +----+--------+ | id | number | +----+--------+ | 1 | 1.0000 | | 2 | 1.0000 | +----+--------+

And numbers can indeed start or end with a period.

Should I use (\\d+)|(\\.\\d+)|(\\d+\\.)|(\\d+\\.\\d+) then, or is there a better way?

This seems to work for the most part: \\b(\\d+(\\.(\\d+)?)?)|(\\.\\d+)\\b
The only problem I can find is that the 3. in 3.a will be tokenized as a number when it shouldn't be. Same with 3a.

winstliu · 2015-10-16T20:54:36Z

In addition, this branch has conflicts now, so if you could rebase, that would be great.

Cxarli · 2015-10-19T13:41:56Z

@50Wliu Rebased

winstliu · 2015-10-19T19:45:42Z

Github is still saying that this branch has conflicts.

Cxarli · 2015-10-19T20:27:10Z

And what are those conflicts? :s

winstliu · 2015-10-19T21:41:22Z

Not sure...how exactly did you rebase?

Cxarli · 2015-10-19T21:42:50Z

Copied all code from my other patch, added the code of this patch, and pushed again

On Mon, Oct 19, 2015 at 11:41 PM, Wliu notifications@github.com wrote:

Not sure...how exactly did you rebase?

Reply to this email directly or view it on GitHub:
#24 (comment)

winstliu · 2015-10-19T21:45:34Z

Copied all code from my other patch, added the code of this patch, and pushed again

Yeah...that won't work. Git won't see that as a resolution to conflicts. What you need to do instead is git pull master, checkout patch-3, and then git merge master. Git will tell you that there are conflicts; then just go in to each file that has conflicts, resolve them, commit, and push.

Cxarli · 2015-10-19T21:46:35Z

I will tomorrow, currently not available to do anything with git

On Mon, Oct 19, 2015 at 11:45 PM, Wliu notifications@github.com wrote:

Copied all code from my other patch, added the code of this patch, and pushed again

Yeah...that won't work. Git won't see that as a resolution to conflicts. What you need to do instead is git pull master, checkout patch-3, and then git merge master. Git will tell you that there are conflicts; then just go in to each file that has conflicts, resolve them, commit, and push.

Reply to this email directly or view it on GitHub:
#24 (comment)

Cxarli · 2015-10-20T08:39:32Z

charlie@chiyuu:~/git/language-sql$ git pull master
fatal: 'master' does not appear to be a git repository
fatal: Could not read from remote repository.

Please make sure you have the correct access rights
and the repository exists.
charlie@chiyuu:~/git/language-sql$ git pull origin
Already up-to-date.
charlie@chiyuu:~/git/language-sql$ git checkout patch-3
Already on 'patch-3'
Your branch is up-to-date with 'origin/patch-3'.
charlie@chiyuu:~/git/language-sql$ git merge master
Already up-to-date.
charlie@chiyuu:~/git/language-sql$ git merge origin
Already up-to-date.

I don't know what I am doing wrong...

Edit: The ~/git/language-sql is from git clone git@github.com:C-Bouthoorn/language-sql.git

winstliu · 2015-10-20T12:55:02Z

Oh whoops, that's because you're on a fork. Try git pull upstream and if that doesn't work, git pull https://github.com/atom/language-sql.git.

winstliu · 2015-10-20T12:55:29Z

If this is all too much work feel free to just close this PR and make a new branch/PR :).

Cxarli · 2015-10-20T14:07:01Z

charlie@chiyuu:~/git/language-sql$ git pull upstream
fatal: 'upstream' does not appear to be a git repository
fatal: Could not read from remote repository.
Please make sure you have the correct access rights
and the repository exists.
charlie@chiyuu:~/git/language-sql$ git pull https://github.com/atom/language-sql.git
From https://github.com/atom/language-sql
 * branch            HEAD       -> FETCH_HEAD
Already up-to-date.

~~Doesn't seem to work either. I'll create a new branch~~

Wrong directory... Think it's fixed now

Conflicts: spec/grammar-spec.coffee

winstliu · 2015-10-20T14:24:07Z

Ok. Last thing before I merge: it doesn't look like you changed the decimal regex yet to support the edge cases .1, 1., etc.

Cxarli · 2015-10-20T14:24:37Z

Oh, I lost that part of code. Let me re-add that

Cxarli · 2015-10-20T14:28:03Z

Also added tests

winstliu · 2015-10-20T14:29:35Z

Will merge once the build goes green 😄.

Cxarli · 2015-10-20T14:33:44Z

Thanks in advantage for all the experience I got by doing this, and sorry if I wasted any of your time. Hope we can meet again in the future 😊

Cxarli · 2015-10-20T15:17:58Z

Looks like Travis is busy...

winstliu · 2015-10-20T15:37:57Z

Haha, yup. I'll just merge this now because the regex and tests both look good. Thanks a lot for sticking with this!

Added decimal highlighting

winstliu · 2015-10-21T02:09:07Z

@C-Bouthoorn specs actually failed, so I just fixed them on master: 05efcd8. For next time, you can run specs locally through apm test or View -> Developer -> Run Package Specs.

Cxarli · 2015-10-21T09:49:34Z

I will, thanks

Update sql.cson

73290d4

winstliu added needs-review requires-changes labels Oct 16, 2015

Update grammar-spec.coffee

f90936f

Adds specs

Update grammar-spec.coffee

02826ff

winstliu reviewed Oct 16, 2015
View reviewed changes

winstliu added under-review and removed needs-review requires-changes labels Oct 16, 2015

winstliu added the requires-changes label Oct 16, 2015

C-Bouthoorn added 2 commits October 17, 2015 12:17

Update sql.cson

5152350

Update grammar-spec.coffee

b564ff2

Merge https://github.com/atom/language-sql into patch-3

e15f7b5

Conflicts: spec/grammar-spec.coffee

C-Bouthoorn added 2 commits October 20, 2015 16:26

Added weird cases

118f361

Added tests for weird cases

f5dce3d

winstliu pushed a commit that referenced this pull request Oct 20, 2015

Merge pull request #24 from C-Bouthoorn/patch-3

e8d02eb

Added decimal highlighting

winstliu merged commit e8d02eb into atom:master Oct 20, 2015

Cxarli deleted the patch-3 branch October 20, 2015 15:41

Cxarli restored the patch-3 branch October 20, 2015 16:08

winstliu mentioned this pull request Nov 20, 2015

Numbers at end of strings aren't highlighted as part of the string #30

Closed

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

Added decimal highlighting #24

Added decimal highlighting #24

Cxarli commented Oct 16, 2015

winstliu commented Oct 16, 2015

Cxarli commented Oct 16, 2015

winstliu commented Oct 16, 2015

Cxarli commented Oct 16, 2015

winstliu commented Oct 16, 2015

Cxarli commented Oct 16, 2015

winstliu Oct 16, 2015

Cxarli Oct 16, 2015

winstliu Oct 16, 2015

winstliu commented Oct 16, 2015

Cxarli commented Oct 19, 2015

winstliu commented Oct 19, 2015

Cxarli commented Oct 19, 2015

winstliu commented Oct 19, 2015

Cxarli commented Oct 19, 2015

Not sure...how exactly did you rebase?

winstliu commented Oct 19, 2015

Cxarli commented Oct 19, 2015

Yeah...that won't work. Git won't see that as a resolution to conflicts. What you need to do instead is `git pull` master, checkout `patch-3`, and then `git merge master`. Git will tell you that there are conflicts; then just go in to each file that has conflicts, resolve them, commit, and push.

Cxarli commented Oct 20, 2015

winstliu commented Oct 20, 2015

winstliu commented Oct 20, 2015

Cxarli commented Oct 20, 2015

winstliu commented Oct 20, 2015

Cxarli commented Oct 20, 2015

Cxarli commented Oct 20, 2015

winstliu commented Oct 20, 2015

Cxarli commented Oct 20, 2015

Cxarli commented Oct 20, 2015

winstliu commented Oct 20, 2015

winstliu commented Oct 21, 2015

Cxarli commented Oct 21, 2015

Added decimal highlighting #24

Added decimal highlighting #24

Conversation

Cxarli commented Oct 16, 2015

winstliu commented Oct 16, 2015

Cxarli commented Oct 16, 2015

winstliu commented Oct 16, 2015

Cxarli commented Oct 16, 2015

winstliu commented Oct 16, 2015

Cxarli commented Oct 16, 2015

winstliu Oct 16, 2015

Choose a reason for hiding this comment

Cxarli Oct 16, 2015

Choose a reason for hiding this comment

winstliu Oct 16, 2015

Choose a reason for hiding this comment

winstliu commented Oct 16, 2015

Cxarli commented Oct 19, 2015

winstliu commented Oct 19, 2015

Cxarli commented Oct 19, 2015

winstliu commented Oct 19, 2015

Cxarli commented Oct 19, 2015

Not sure...how exactly did you rebase?

winstliu commented Oct 19, 2015

Cxarli commented Oct 19, 2015

Yeah...that won't work. Git won't see that as a resolution to conflicts. What you need to do instead is git pull master, checkout patch-3, and then git merge master. Git will tell you that there are conflicts; then just go in to each file that has conflicts, resolve them, commit, and push.

Cxarli commented Oct 20, 2015

winstliu commented Oct 20, 2015

winstliu commented Oct 20, 2015

Cxarli commented Oct 20, 2015

winstliu commented Oct 20, 2015

Cxarli commented Oct 20, 2015

Cxarli commented Oct 20, 2015

winstliu commented Oct 20, 2015

Cxarli commented Oct 20, 2015

Cxarli commented Oct 20, 2015

winstliu commented Oct 20, 2015

winstliu commented Oct 21, 2015

Cxarli commented Oct 21, 2015

Yeah...that won't work. Git won't see that as a resolution to conflicts. What you need to do instead is `git pull` master, checkout `patch-3`, and then `git merge master`. Git will tell you that there are conflicts; then just go in to each file that has conflicts, resolve them, commit, and push.