Skip to content
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

Stronger check on blocks names #6295

Closed
wants to merge 2 commits into from

Conversation

fpoulain
Copy link

@fpoulain fpoulain commented Aug 5, 2020

Ref: wagtail-deprecated/wagtail-react-streamfield#54

This PR stronger check on block names as stated in the docs.

‘name’ is used to identify the block type within templates and the internal JSON representation (and should follow standard Python conventions for variable names: lower-case and underscores, no spaces)

Since it may break existing code I made use of a warning rather than an error. Let me know how do you think about it.
Maybe only in a second phase it should become a strong error and could replace others name checks.

Thanks for Wagtail! 🎉

@squash-labs
Copy link

squash-labs bot commented Aug 5, 2020

Manage this branch in Squash

Test this branch here: https://fpoulainstricter-block-check-n-8aytm.squash.io

@khashashin
Copy link

khashashin commented Aug 6, 2020

Thanks for this, I lost yesterday so much time because I didn't read the docs carefully and have defined the name with brackets: cards_(_left_side_icons_). There wasn't any warning or errors during migrations, and only checking the front end of wagtail admin interface displayed the error in the console.

@gasman
Copy link
Collaborator

gasman commented Aug 6, 2020

Thanks @fpoulain! Code looks good to me.

I think this should be an error, not just a warning. There's at least one place in the design of StreamField where the block name needs to be a valid Python identifier - namely, being able to write {{ value.some_child_block }} within templates for StructBlocks - so letting developers use invalid names to a limited extent isn't really doing them any favours.

@fpoulain
Copy link
Author

fpoulain commented Aug 6, 2020

I think this should be an error, not just a warning.

Ok I will change this.

What about the redondancy of the errors? The first check (name empty) could be left as is but the others (dash/space/numeric) could be removed since they are covered by my check.

@gasman
Copy link
Collaborator

gasman commented Aug 6, 2020

The redundancy of the checks did cross my mind, but it's probably a good thing that the more common mistakes (using spaces / dashes) give you a more focused error message telling you exactly what to change, rather than just "you must follow this format". So, I think this can stay as it is 👍

@fpoulain
Copy link
Author

fpoulain commented Aug 6, 2020

Ok. I updated the PR. I let you commit the changelog because my english is too approximative. Moreover, since it will breaks code for some users which --like me-- didn't read carefully the docs, imho this change should be told in the release notes.

@gasman
Copy link
Collaborator

gasman commented Sep 28, 2020

Merged in 0c68159.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

None yet

3 participants