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 'transform' config to match-exports rule #9

Merged
merged 3 commits into from Jul 13, 2016

Conversation

Projects
None yet
4 participants
@scottnonnenberg
Contributor

scottnonnenberg commented Jun 7, 2016

Documentation included! :0)

scottnonnenberg added some commits Jun 7, 2016

@coveralls

This comment has been minimized.

coveralls commented Jun 7, 2016

Coverage Status

Coverage remained the same at 100.0% when pulling 2d4e71d on scottnonnenberg:add-export-transforms into 721ea9d on selaux:master.

scottnonnenberg added a commit to scottnonnenberg/eslint-config-thehelp that referenced this pull request Jun 9, 2016

@selaux

This comment has been minimized.

Owner

selaux commented Jun 21, 2016

Thanks for the PR. I'm not sure though if this is actually a common use case. Additionally having a predefined set of these matching rules looks weird to me (i.e. why do we only support exacty these cases). So I would rather wait to merge until more people chive in.

@scottnonnenberg

This comment has been minimized.

Contributor

scottnonnenberg commented Jun 21, 2016

Hm. I can certainly say that I can't use it without the changes - I'm not willing to introduce case-sensitivity into my project filenames, having been bitten going from dev to production in the past.

I did take some time to consider configurability. You're right, that set of three is limited, and it is a subset of what people might want. Of course, those are the first three options I can think of when transforming an identifier, so there's that. Could always introduce the ability to provide a custom transform, but that would only be available to people providing their configuration in javascript.

To me, this change made sense given the configurability of that primary rule in this plugin: match-regex. I got that all working to my needs, then discovered the match-exports rule and realized that I couldn't go any further without code changes.

@kav

This comment has been minimized.

kav commented Jul 8, 2016

+1, I don't use camel case filenames so the default configuration of the match-exported rule is useless without @scottnonnenberg's changes. The use case is foo-bar.js should export FooBar that's a pretty straightforward and common use case AFAIK. Given he has covered the standard naming conventions it seems to be flexible enough. Are there more standard naming conventions you'd expect to cover?

In general in fact I'd love a rule that offers those options to enforce these filename patterns over an arbitrary regex. I don't need infinite flexibility regex provides here. I need consistency with common best practices. Perhaps you'd disagree but I'd argue it's ok for linter rules to proscribe a set of options based on common coding patterns.

@selaux

This comment has been minimized.

Owner

selaux commented Jul 12, 2016

You're right. It might be a common use case for existing code bases (especially for projects developed on OS X and Windows and deployed to Linux). The issue I see with flexibility is with rather fringe use-cases like LinkComponent being allowed in ./components/Link, but I'd rather not cover these than making the plugin unusable for you guys. So now this is about getting the PR ready to merge 🍻

@@ -24,6 +27,25 @@ function getStringToCheckAgainstExport(parsed) {
}
}
function transform(exportedName, context) {

This comment has been minimized.

@selaux

selaux Jul 12, 2016

Owner

What about:

/// ...
var transforms = {
    kebab: kebabCase,
    snake: snakeCase,
    camel: camelCase
};
// ...

function transform(exportedName, transformName) {
    var transform = transforms[transformName];

    return transform ? transform(exportedName) : exportedName;
}

This way the exported name can always be passed to this function regardless of wether a transform option is actually passed.

@@ -34,16 +56,18 @@ module.exports = function(context) {
exportedName = getExportedName(node),
isExporting = Boolean(exportedName),
expectedExport = getStringToCheckAgainstExport(parsed),
transformed = transform(exportedName, context),
transformedText = transformed ? ' (transformed)' : '',

This comment has been minimized.

@selaux

selaux Jul 12, 2016

Owner

This can probably always be (transformed) since the user clearly sees both values and can determine whether they are transformed or not.

everythingIsIndex = exportedName === 'index' && parsed.name === 'index',
matchesExported = expectedExport === exportedName || everythingIsIndex,
matchesExported = (transformed ? transformed === expectedExport : expectedExport === exportedName) || everythingIsIndex,

This comment has been minimized.

@selaux

selaux Jul 12, 2016

Owner

With above this can always be transformed === expectedExport

@@ -135,3 +135,94 @@ ruleTester.run("lib/rules/match-exported", exportedRule, {
}
]
});
var camelCaseCommonJS = "module.exports = variableName;",

This comment has been minimized.

@selaux

selaux Jul 12, 2016

Owner

These should be at the top of the file (I probably should update the projects linting rules 😉 )

);
}
}
};
module.exports.schema = [

This comment has been minimized.

@selaux

selaux Jul 12, 2016

Owner

Can we keep this as an array for now to be consistent with the other rules? Although I also like the object version, but then it should be there for all rules (and thus requires a major update).

@scottnonnenberg

This comment has been minimized.

Contributor

scottnonnenberg commented Jul 13, 2016

That should address all your feedback!

@coveralls

This comment has been minimized.

coveralls commented Jul 13, 2016

Coverage Status

Coverage remained the same at 100.0% when pulling 08bcdbd on scottnonnenberg:add-export-transforms into 721ea9d on selaux:master.

@scottnonnenberg

This comment has been minimized.

Contributor

scottnonnenberg commented Jul 13, 2016

Failures on platforms below Node 4.x are due to the * version specified for eslint: http://eslint.org/blog/2016/07/eslint-v3.0.0-released

@selaux

This comment has been minimized.

Owner

selaux commented Jul 13, 2016

Thanks for the rework 👍. I will adress the eslint issue on master.

@selaux selaux merged commit 6578756 into selaux:master Jul 13, 2016

1 check failed

continuous-integration/travis-ci/pr The Travis CI build failed
Details

@scottnonnenberg scottnonnenberg referenced this pull request Feb 12, 2018

Closed

Auto-orient images based on EXIF metadata #2040

18 of 21 tasks complete
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment