-
-
Notifications
You must be signed in to change notification settings - Fork 2.1k
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
Add ignore option to component generation. #490
Conversation
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
looks good 💃
@@ -36,8 +42,7 @@ function writeError(msg, filePath) { | |||
} | |||
|
|||
function checkWarn(name, value) { | |||
const excluded = ['setProps', 'id', 'className', 'style', 'dashEvents', 'fireEvent']; | |||
if (value.length < 1 && !excluded.includes(name)) { | |||
if (value.length < 1 && !excludedDocProps.includes(name.split('.').pop())) { |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Small change in logic here with the name
becoming name.split
, can you explain what scenario this fixes?
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
The name is actually the Component name + prop name and we only want the prop name.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Kk thx
@@ -22,14 +22,17 @@ class _CombinedFormatter(argparse.ArgumentDefaultsHelpFormatter, | |||
|
|||
# pylint: disable=too-many-locals | |||
def generate_components(components_source, project_shortname, | |||
package_info_filename='package.json'): | |||
package_info_filename='package.json', | |||
ignore='^_'): |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
This covers the __tests__
case mentioned in #486 but maybe a small set of defaults (e.g [__tests__, tests]
) to cover both the dash-bootstrap-components and our own dash-component-boilerplate case would make sense. Surely there is never a case where this is desirable.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
This is an additional feature and should be documented in the changelog and the version be bumped.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Version bump and defaults comment but otherwise seems good.
37ba9b2
to
fd096a9
Compare
Adds an ignore option to
dash-generate-components
cli, default is^_
so it will ignore the__tests__
folder pattern by default.Closes #486