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

Parser: Tabs or spaces? This never should have gotten in #10379

Merged
merged 1 commit into from Oct 10, 2018

Conversation

Projects
None yet
5 participants
@dmsnell
Contributor

dmsnell commented Oct 6, 2018

Oops ¯\(ツ)

well I was waiting until I could do this in one fell swoop instead of mixing it in with actual changes. there should be no functional, behavioral, or visual changes in this.

@dmsnell dmsnell requested review from mcsf, mtias and aduth Oct 6, 2018

@chrisvanpatten

This comment has been minimized.

Show comment
Hide comment
@chrisvanpatten

chrisvanpatten Oct 7, 2018

Contributor

This file could probably be added into the phpcs config so it'll be caught next time!

https://github.com/WordPress/gutenberg/blob/master/phpcs.xml.dist#L24

Contributor

chrisvanpatten commented Oct 7, 2018

This file could probably be added into the phpcs config so it'll be caught next time!

https://github.com/WordPress/gutenberg/blob/master/phpcs.xml.dist#L24

@dmsnell

This comment has been minimized.

Show comment
Hide comment
@dmsnell

dmsnell Oct 9, 2018

Contributor

Thanks @chrisvanpatten - added in 3a24898 - is that all I need to do?

Contributor

dmsnell commented Oct 9, 2018

Thanks @chrisvanpatten - added in 3a24898 - is that all I need to do?

@mcsf mcsf added this to the 4.0 milestone Oct 9, 2018

@mcsf

mcsf approved these changes Oct 9, 2018

Assuming CI will go green and assuming Chris's 👍, this looks good to go.

@gziolo

This comment has been minimized.

Show comment
Hide comment
@gziolo

gziolo Oct 9, 2018

Member

@pento can you double check that we lint all PHP files after we moved everything to packages folder?

Member

gziolo commented Oct 9, 2018

@pento can you double check that we lint all PHP files after we moved everything to packages folder?

@chrisvanpatten

This comment has been minimized.

Show comment
Hide comment
@chrisvanpatten

chrisvanpatten Oct 9, 2018

Contributor

Looks good to me 👍

Contributor

chrisvanpatten commented Oct 9, 2018

Looks good to me 👍

@dmsnell

This comment has been minimized.

Show comment
Hide comment
@dmsnell

dmsnell Oct 9, 2018

Contributor

@chrisvanpatten these lint rules are pretty mismatched with this module. I'm struggling to decide…

  • exclude this file from phpcs as it was originally
  • disable certain rules for the entire file (to prevent things like saying innerHTML is invalid)
  • disable rules inline and litter the file with exceptions
  • create a new XML config section for phpcs that excludes things file-wide

any thoughts? if not, how about we remove this from the phpcs config in this PR and do it in a separate PR?

Contributor

dmsnell commented Oct 9, 2018

@chrisvanpatten these lint rules are pretty mismatched with this module. I'm struggling to decide…

  • exclude this file from phpcs as it was originally
  • disable certain rules for the entire file (to prevent things like saying innerHTML is invalid)
  • disable rules inline and litter the file with exceptions
  • create a new XML config section for phpcs that excludes things file-wide

any thoughts? if not, how about we remove this from the phpcs config in this PR and do it in a separate PR?

@chrisvanpatten

This comment has been minimized.

Show comment
Hide comment
@chrisvanpatten

chrisvanpatten Oct 9, 2018

Contributor

@dmsnell Aside from the tedium of making the changes is there any technical reason this class should be exempted from the WPCS rules? It's flagging a lot of things but none of them strike me as unreasonable.

I can appreciate wanting to keep the JS values consistent with PHP (with things like innerHTML) but otherwise it seems like the rest could be fixed.

Happy to take over and fix as much as I can, if you'd like…

Contributor

chrisvanpatten commented Oct 9, 2018

@dmsnell Aside from the tedium of making the changes is there any technical reason this class should be exempted from the WPCS rules? It's flagging a lot of things but none of them strike me as unreasonable.

I can appreciate wanting to keep the JS values consistent with PHP (with things like innerHTML) but otherwise it seems like the rest could be fixed.

Happy to take over and fix as much as I can, if you'd like…

@mcsf

This comment has been minimized.

Show comment
Hide comment
@mcsf

mcsf Oct 9, 2018

Contributor

Just to keep momentum, I suggest getting the original whitespace-only changes in now, and deal with config and linting fixes subsequently.

Contributor

mcsf commented Oct 9, 2018

Just to keep momentum, I suggest getting the original whitespace-only changes in now, and deal with config and linting fixes subsequently.

@dmsnell

This comment has been minimized.

Show comment
Hide comment
@dmsnell

dmsnell Oct 9, 2018

Contributor

@chrisvanpatten definitely not asking for this to be exempted, though I am questioning how to apply the rules best and if they ought to be exempted.

Aside from the tedium of making the changes

the tedium of making the changes is completely absent from my thought process. I can make the changes faster than I can tie my shoes 🙃

it's more like in this case, I'm trying to do a cleanup on this file and get it moving forward and ready to make more cleanups, but after adding it to phpcs now I can't even get that first step out the door so the further progress is being frustrated by it

none of them strike me as unreasonable.

what's unreasonable is that the fixes I'm making to appease the linter aren't making the code any better.

this is unsatisfactory:
// otherwise we found an inner block

this is satisfactory:
// otherwise we found an inner block.

this is wrong:
public $innerBlocks;

this is right:
public $innerBlocks; // phpcs:ignore WordPress.NamingConventions.ValidVariableName.MemberNotSnakeCase -- matching spec

once I started adding the comments to ignore the rules that don't apply it started making the code harder to understand for me and annoying to copy and paste, plus it irks me that we're so tightly mixing the code which implements a parser specification and code hidden in comments which interacts with linting (thus the question about a separate set of XML rules for phpcs that lives outside of this file).


mainly I think I'd like to agree with @mcsf if you are willing to remove the phpcs part of this PR and leave that for a separate issue where we can work through the implications of the linting and if-needed make deeper changes to fulfill the intention behind the rules vs. just adding noise to the code to trick the linter into thinking we made real updates 😄

thoughts?

Contributor

dmsnell commented Oct 9, 2018

@chrisvanpatten definitely not asking for this to be exempted, though I am questioning how to apply the rules best and if they ought to be exempted.

Aside from the tedium of making the changes

the tedium of making the changes is completely absent from my thought process. I can make the changes faster than I can tie my shoes 🙃

it's more like in this case, I'm trying to do a cleanup on this file and get it moving forward and ready to make more cleanups, but after adding it to phpcs now I can't even get that first step out the door so the further progress is being frustrated by it

none of them strike me as unreasonable.

what's unreasonable is that the fixes I'm making to appease the linter aren't making the code any better.

this is unsatisfactory:
// otherwise we found an inner block

this is satisfactory:
// otherwise we found an inner block.

this is wrong:
public $innerBlocks;

this is right:
public $innerBlocks; // phpcs:ignore WordPress.NamingConventions.ValidVariableName.MemberNotSnakeCase -- matching spec

once I started adding the comments to ignore the rules that don't apply it started making the code harder to understand for me and annoying to copy and paste, plus it irks me that we're so tightly mixing the code which implements a parser specification and code hidden in comments which interacts with linting (thus the question about a separate set of XML rules for phpcs that lives outside of this file).


mainly I think I'd like to agree with @mcsf if you are willing to remove the phpcs part of this PR and leave that for a separate issue where we can work through the implications of the linting and if-needed make deeper changes to fulfill the intention behind the rules vs. just adding noise to the code to trick the linter into thinking we made real updates 😄

thoughts?

@dmsnell

This comment has been minimized.

Show comment
Hide comment
@dmsnell

dmsnell Oct 9, 2018

Contributor

reverting HEAD to first commit, old HEAD was d0670d1

Contributor

dmsnell commented Oct 9, 2018

reverting HEAD to first commit, old HEAD was d0670d1

Parser: Tabs or spaces? This never should have gotten in
Oops ¯\(ツ)/¯

well I was waiting until I could do this in one fell swoop instead of
mixing it in with actual changes. there should be no functional,
behavioral, or visual changes in this.
@chrisvanpatten

This comment has been minimized.

Show comment
Hide comment
@chrisvanpatten

chrisvanpatten Oct 9, 2018

Contributor

Definitely don't consider my opinions as blocking!

what's unreasonable is that the fixes I'm making to appease the linter aren't making the code any better.

this is unsatisfactory:
// otherwise we found an inner block

this is satisfactory:
// otherwise we found an inner block.

Sure, and we could bikeshed on this all day, but at the end of the day they are the WordPress coding standards, this is a WordPress project, and these are the standards that the WordPress project has decided to adopt. They are definitely aggressively prescriptive (I personally use PSR-2 on my personal projects because I don't really like WPCS) but they are the standards we have and as a core project it seems like this is a case where they should be used.

But I personally have no problem at all with tackling that in a separate PR; I'm even happy to do it myself after this is merged :)

Contributor

chrisvanpatten commented Oct 9, 2018

Definitely don't consider my opinions as blocking!

what's unreasonable is that the fixes I'm making to appease the linter aren't making the code any better.

this is unsatisfactory:
// otherwise we found an inner block

this is satisfactory:
// otherwise we found an inner block.

Sure, and we could bikeshed on this all day, but at the end of the day they are the WordPress coding standards, this is a WordPress project, and these are the standards that the WordPress project has decided to adopt. They are definitely aggressively prescriptive (I personally use PSR-2 on my personal projects because I don't really like WPCS) but they are the standards we have and as a core project it seems like this is a case where they should be used.

But I personally have no problem at all with tackling that in a separate PR; I'm even happy to do it myself after this is merged :)

@dmsnell

This comment has been minimized.

Show comment
Hide comment
@dmsnell

dmsnell Oct 9, 2018

Contributor

Yeah definitely don't consider my opinions blocking.

Thanks! I'll merge as soon as the tests to finish green then - some unexpected issue came up and I don't know why (the diff is now empty when ignoring whitespace)

they are the standards we have and as a core project it seems like this is a case where they should be used.

the point is that we don't just need a . or a ? - having a period isn't the core standard. the standard is having something as a sentence that's meaningful. adding a . is fast and breaks the spirit of the rule but making the necessary changes is going to take more thought and time.

it's not bike shedding unless we're arguing about meeting the letter of the law while ignoring the reason the law exists in the first place 😉

But I personally have no problem at all with tackling that in a separate PR; I'm even happy to do it myself after this is merged :)

would be awesome. I also don't mind. also, if you do, would you mind pinging me on the PRs as well? if there are any questions I'd be happy to share insight behind the code. also, I'm still not convinced that adding a bunch of inline phpcs comments is the right decision. for example, can we tell it to allow innerHTML and innerBlocks and blockName throughout the file? I think that we need to hold the contract of these names to prevent adding confusion to the parsing system.

Contributor

dmsnell commented Oct 9, 2018

Yeah definitely don't consider my opinions blocking.

Thanks! I'll merge as soon as the tests to finish green then - some unexpected issue came up and I don't know why (the diff is now empty when ignoring whitespace)

they are the standards we have and as a core project it seems like this is a case where they should be used.

the point is that we don't just need a . or a ? - having a period isn't the core standard. the standard is having something as a sentence that's meaningful. adding a . is fast and breaks the spirit of the rule but making the necessary changes is going to take more thought and time.

it's not bike shedding unless we're arguing about meeting the letter of the law while ignoring the reason the law exists in the first place 😉

But I personally have no problem at all with tackling that in a separate PR; I'm even happy to do it myself after this is merged :)

would be awesome. I also don't mind. also, if you do, would you mind pinging me on the PRs as well? if there are any questions I'd be happy to share insight behind the code. also, I'm still not convinced that adding a bunch of inline phpcs comments is the right decision. for example, can we tell it to allow innerHTML and innerBlocks and blockName throughout the file? I think that we need to hold the contract of these names to prevent adding confusion to the parsing system.

@dmsnell dmsnell merged commit 1349d23 into master Oct 10, 2018

2 checks passed

codecov/project 49.5% remains the same compared to 32f763c
Details
continuous-integration/travis-ci/pr The Travis CI build passed
Details

@dmsnell dmsnell deleted the update/parser-consistent-whitespace branch Oct 10, 2018

@aduth

This comment has been minimized.

Show comment
Hide comment
@aduth

aduth Oct 10, 2018

Member

What was the original pull request which introduced the issue? Curious to find more context on why we want to allow this exception:

this is wrong:
public $innerBlocks;

this is right:
public $innerBlocks; // phpcs:ignore
WordPress.NamingConventions.ValidVariableName.MemberNotSnakeCase -- matching spec

Member

aduth commented Oct 10, 2018

What was the original pull request which introduced the issue? Curious to find more context on why we want to allow this exception:

this is wrong:
public $innerBlocks;

this is right:
public $innerBlocks; // phpcs:ignore
WordPress.NamingConventions.ValidVariableName.MemberNotSnakeCase -- matching spec

@dmsnell

This comment has been minimized.

Show comment
Hide comment
@dmsnell

dmsnell Oct 10, 2018

Contributor

#8083 introduced it @aduth

the parser specification defines the variable names and they were chosen to be the same in JS as in PHP to enforce consistency. the idea is that nobody building a new implementation should have to ask how to change variable names based on language idioms - the contract is a JSON-ish array of block objects

in other words, if you parse a document in the server it should produce identical output as parsing it in the browser would

Contributor

dmsnell commented Oct 10, 2018

#8083 introduced it @aduth

the parser specification defines the variable names and they were chosen to be the same in JS as in PHP to enforce consistency. the idea is that nobody building a new implementation should have to ask how to change variable names based on language idioms - the contract is a JSON-ish array of block objects

in other words, if you parse a document in the server it should produce identical output as parsing it in the browser would

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment