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

Add ignore option to component generation. #490

Merged
merged 2 commits into from
Dec 18, 2018
Merged

Conversation

T4rk1n
Copy link
Contributor

@T4rk1n T4rk1n commented Dec 13, 2018

Adds an ignore option to dash-generate-components cli, default is ^_ so it will ignore the __tests__ folder pattern by default.

Closes #486

@T4rk1n T4rk1n requested a review from rmarren1 December 13, 2018 21:19
Copy link
Contributor

@rmarren1 rmarren1 left a 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())) {
Copy link
Contributor

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?

Copy link
Contributor Author

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.

Copy link
Contributor

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='^_'):
Copy link
Contributor

@Marc-Andre-Rivet Marc-Andre-Rivet Dec 14, 2018

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.

Copy link
Contributor

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.

Copy link
Contributor

@Marc-Andre-Rivet Marc-Andre-Rivet left a 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.

@T4rk1n T4rk1n force-pushed the improve-generation-cli branch from 37ba9b2 to fd096a9 Compare December 18, 2018 00:00
@T4rk1n T4rk1n merged commit 0c7eb4a into master Dec 18, 2018
@T4rk1n T4rk1n deleted the improve-generation-cli branch December 18, 2018 00:04
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.

3 participants