Skip to content

Conversation

@aduth
Copy link
Member

@aduth 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 added the [Feature] Parsing Related to efforts to improving the parsing of a string of data and converting it into a different f label Nov 16, 2017
@aduth aduth requested a review from dmsnell November 16, 2017 16:20
@codecov
Copy link

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.

ASCII_AlphaNumeric
= ASCII_Letter
ASCII_LowercaseAlphaNumeric
= ASCII_LowercaseLetter
Copy link
Member

@dmsnell dmsnell Nov 16, 2017

Choose a reason for hiding this comment

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

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_-]*)

Copy link
Member Author

Choose a reason for hiding this comment

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

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.

Copy link
Member

Choose a reason for hiding this comment

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

looks nice!

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

Labels

[Feature] Parsing Related to efforts to improving the parsing of a string of data and converting it into a different f

Projects

None yet

Development

Successfully merging this pull request may close these issues.

3 participants