Fix comment parsing #123

Merged
merged 2 commits into from Jan 20, 2017

Projects

None yet

4 participants

@bigfoot90
Contributor

Related with #120 (comment)
The Lexer adds a \n at the end of each comment.

@bigfoot90 bigfoot90 Fix comment parsing
4bef088
@codecov-io
codecov-io commented Jan 8, 2017 edited

Current coverage is 99.92% (diff: 100%)

Merging #123 into master will decrease coverage by <.01%

@@             master       #123   diff @@
==========================================
  Files            53         53          
  Lines          3771       3755    -16   
  Methods         184        184          
  Messages          0          0          
  Branches          0          0          
==========================================
- Hits           3768       3752    -16   
  Misses            3          3          
  Partials          0          0          

Powered by Codecov. Last update 2a589a3...5127520

@bigfoot90 bigfoot90 referenced this pull request Jan 8, 2017
Merged

Better Formatter tests #120

@bigfoot90 bigfoot90 Fix comment parsing
5127520
*/
$curr = $list->tokens[$list->idx];
if ($curr->type === Token::TYPE_WHITESPACE) {
// Whitespaces are skipped because the formatter adds its own.
continue;
- } elseif ($curr->type === Token::TYPE_COMMENT) {
@bigfoot90
bigfoot90 Jan 10, 2017 Contributor

@nijel Do you know to what these lines was made for? Formatter works much better now.

@nijel
nijel Jan 17, 2017 Member

It's really there since beginning (e6a562e), most likely to behave consistently with formatter we had in past in phpMyAdmin. Maybe @udan11 can comment on this...

@bigfoot90
Contributor
bigfoot90 commented Jan 10, 2017 edited

Ready for review! ;-)

Input

SELECT /* Comment */ 1
FROM tbl # Comment
WHERE 1 -- Comment

Before

SELECT
     /* Comment */ 1
FROM
    tbl
 # Comment\x0AWHERE
    1 -- Comment\x0A

Now

SELECT
    /* Comment */ 1
FROM
    tbl # Comment
WHERE
    1 -- Comment

Also written test to prevent regression.

@bigfoot90
Contributor

Should I rebase commits?

@ibennetch
Contributor

I'll let Michal or one of the others comment on the code here, but as far as your rebase question there are no conflicts currently so there's no need to rebase at this time.

A rebase would be handy if there were commits to 'master' that conflicted with your work or if you wished to keep your branch up to date with some other changes that have been happening in master (such as happens in main phpMyAdmin with a branch that takes a while to develop and merge back in, it's handy to rebase to stay up-to-date), but in this case I don't see a need.

@bigfoot90
Contributor

Should I rebase commits?
Should I squash commits?

@bigfoot90
Contributor

@nijel Any news on this?

@ibennetch
Contributor

Nijel may be able to squash the commits when merging (Github has added some feature for this recently), but he'll let you know what would be best.

Since neither he nor anyone else has commented, and since I'm still not very familiar with the code in the parser, I suggest to must've patient because someone will get to this soon.

Thanks!

@bigfoot90
Contributor
bigfoot90 commented Jan 15, 2017 edited

Sorry, I know you are busy and full of work, I did not mean to hurry.

@ibennetch
Contributor
@nijel
Member
nijel commented Jan 17, 2017

Overall I think your change makes sense, let's give @udan11 some time to comment on this as he is author of this code and there might have been some reasons behind it.

@nijel nijel self-assigned this Jan 17, 2017
@bigfoot90
Contributor

Do you have any update?

@nijel nijel merged commit ed8d676 into phpmyadmin:master Jan 20, 2017

4 checks passed

codacy/pr Good work! A positive pull request.
Details
codecov/patch 100% of diff hit (target 99.92%)
Details
codecov/project Absolute coverage decreased by -<.01% but relative coverage increased by +0.07% compared to 2a589a3
Details
continuous-integration/travis-ci/pr The Travis CI build passed
Details
@nijel
Member
nijel commented Jan 20, 2017

Okay, let's merge it as it looks reasonable to me. In case @udan11 disagrees later, we can handle that.

@bigfoot90
Contributor

Thanks

@bigfoot90 bigfoot90 deleted the bigfoot90:fix-comment-parsing branch Jan 20, 2017
@nijel nijel added a commit that referenced this pull request Jan 20, 2017
@nijel nijel Add changelog entry for #123
Signed-off-by: Michal Čihař <michal@cihar.com>
23962cf
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment