Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

Already on GitHub? Sign in to your account

Parsing: Enforce block naming requirements consistently #3521

Merged
merged 2 commits into from Nov 17, 2017

Conversation

Projects
None yet
2 participants
Member

aduth commented Nov 16, 2017

This pull request seeks to resolve inconsistencies between block registration validation and name parsing expectations. Specifically:

  • Registration requires that a block name be lowercase, but this was not enforced by the block parser
  • The block parser enforces that a block name start with a letter, but this was not validated by registration

Further, these requirements were not documented in block registration documentation.

The proposed changes remedies each of the above issues.

Testing instructions:

Ensure tests pass:

npm test

Follow-up Tasks:

Gutenberg examples have names beginning with numbers and must be updated:

https://github.com/WordPress/gutenberg-examples

@aduth aduth requested a review from dmsnell Nov 16, 2017

codecov bot commented Nov 16, 2017

Codecov Report

Merging #3521 into master will increase coverage by 1.08%.
The diff coverage is 100%.

Impacted file tree graph

@@            Coverage Diff             @@
##           master    #3521      +/-   ##
==========================================
+ Coverage   34.54%   35.62%   +1.08%     
==========================================
  Files         261      262       +1     
  Lines        6710     6896     +186     
  Branches     1225     1286      +61     
==========================================
+ Hits         2318     2457     +139     
- Misses       3704     3738      +34     
- Partials      688      701      +13
Impacted Files Coverage Δ
blocks/api/registration.js 100% <100%> (ø) ⬆️
blocks/library/shortcode/index.js 33.33% <0%> (-6.67%) ⬇️
blocks/library/html/index.js 14.28% <0%> (-2.39%) ⬇️
blocks/library/more/index.js 20% <0%> (-2.23%) ⬇️
blocks/hooks/index.js 100% <0%> (ø) ⬆️
editor/writing-flow/index.js 0% <0%> (ø) ⬆️
editor/store-persist.js 100% <0%> (ø) ⬆️
blocks/hooks/custom-class-name.js 84.61% <0%> (ø)
blocks/api/parser.js 98.48% <0%> (+0.4%) ⬆️
blocks/library/list/index.js 7.31% <0%> (+0.42%) ⬆️
... and 8 more

Continue to review full report at Codecov.

Legend - Click here to learn more
Δ = absolute <relative> (impact), ø = not affected, ? = missing data
Powered by Codecov. Last update e7aaa99...df43eb4. Read the comment docs.

blocks/api/post.pegjs
-ASCII_AlphaNumeric
- = ASCII_Letter
+ASCII_LowercaseAlphaNumeric
+ = ASCII_LowercaseLetter
@dmsnell

dmsnell Nov 16, 2017

Collaborator

this seems a bit verbose. what if instead we ripped out the whole ASCII nomenclature and just simplified down to something like Block_Name or Block_Name_Part - we're not using the other rules so we can get rid of them

Namespaced_Block_Name
  = $( Block_Name_Part "/" Block_Name_Part )

Core_Block_Name
  = type:Block_Name_Part { return 'core/' + type }

Block_Name_Part
  = $([a-z] [a-z0-9_-]*)
@aduth

aduth Nov 17, 2017

Member

this seems a bit verbose. what if instead we ripped out the whole ASCII nomenclature and just simplified down to something like Block_Name or Block_Name_Part

Sounds good to me. Applied in df43eb4.

@dmsnell

dmsnell Nov 17, 2017

Collaborator

looks nice!

@aduth aduth merged commit c5d19a6 into master Nov 17, 2017

3 checks passed

codecov/project 35.62% (+1.08%) compared to e7aaa99
Details
continuous-integration/travis-ci/pr The Travis CI build passed
Details
continuous-integration/travis-ci/push The Travis CI build passed
Details

@aduth aduth deleted the update/consistent-block-name-requirements branch Nov 17, 2017

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