selaux / eslint-plugin-filenames

Eslint plugin to check filenames.
318 stars 35 forks source link

Improvement on 'match-regex-and-exported' #7

Closed call-a3 closed 8 years ago

call-a3 commented 8 years ago

The rule should also accept the filename if it is 'index' and the parent directory name matches the regex and/or exported name. This is in compliance with the default behaviour of require to use the index file if a directory name is required/imported.

selaux commented 8 years ago

Sounds good, will try to do this in the future.

call-a3 commented 8 years ago

Thanks, I appreciate that.

selaux commented 8 years ago

Can you retest current master and see if this implementation works for you?

cloudkite commented 8 years ago

@selaux great addition! Have tried it out on a code base with alot of index files and works great!

call-a3 commented 8 years ago

I get an error stating The directory 'index' must be named 'state', after the exported value of its index file.. However, the directory's name is 'state', which is also the name of the default exported function in index.js.

My configuration is

rules:  
    filenames/filenames:  
      - 2  
      - ^[a-zA-Z]+$  
      - match-regex-and-exported  
      - true  

Am I doing something wrong or is this a bug in the functionality?

selaux commented 8 years ago

Config looks fine. Can you post the absolute paths of your files?

cloudkite commented 8 years ago

@selaux I get this error as well but only if running eslint through atom plugin linter-eslint. Running via gulp-eslint I get the correct result.

I think this is a result of linter changing the working directory before linting files and therefore parsed.dir is an empty string. If I change the first couple of lines of getStringToCheckAgainstExport to the following it works fine

function getStringToCheckAgainstExport(parsed) {
    var dir = parsed.dir.length ? parsed.dir : process.cwd();
    var dirArray = dir.split(path.sep);
selaux commented 8 years ago

I will have a look at it, thanks for reporting. Even better before the release so I dont have to do a bugfix release :+1:.

call-a3 commented 8 years ago

I'm also using this with atom's linter-eslint, so I'm probably affected by the same bug. But if you want I can still provide a full writeup of my directory structure.

selaux commented 8 years ago

Can you please retest current master?

call-a3 commented 8 years ago

Tested it, seems to work now.

Met vriendelijke groet, Adriaan Callaerts

2016-03-14 19:49 GMT+01:00 Stefan Lau notifications@github.com:

Can you please retest current master?

— Reply to this email directly or view it on GitHub https://github.com/selaux/eslint-plugin-filenames/issues/7#issuecomment-196466516 .

mlucool commented 8 years ago

Testing against master, this does not seem to work for in the following case:

"filenames/filenames": [2, "^([a-z][-a-zA-Z_0-9]+)|([a-zA-Z][-a-zA-Z_0-9]+_spec)$", "match-exported-or-regex"]

This rule says if default export use that, if not, camelCase (except if its a _spec).

Example error 1 (which works as expected): /foo/Bar/index.js contains const Baz = {}; export default Baz; Example error 2 (which does not error when I expect it to): /foo/Bar/index.js contains const Baz = {}; export Baz;

In the second case, Bar error for not being camelCase as there is no default export and there is an index.js.

selaux commented 8 years ago

@mlucool I added this testcase corresponding to your usecase and it produces the error just fine. Can you double check wether I missed something?

        {
            code: "export const Baz = {};",
            filename: "/some/dir/Bar/index.js",
            options: [ "^a$", "match-exported-or-regex" ],
            parserOptions: { ecmaVersion: 6, sourceType: "module" },
            errors: [
                { message: "Filename 'index.js' does not match the naming convention.", column: 1, line: 1 }
            ]
        }
mlucool commented 8 years ago

Try this:

        {
            code: "export const Baz = {};",
            filename: "/some/dir/Bar/index.js",
            options: [ "^[a-z]+$", "match-exported-or-regex" ],
            parserOptions: { ecmaVersion: 6, sourceType: "module" },
            errors: [
                { message: "Filename 'index.js' does not match the naming convention.", column: 1, line: 1 }
            ]
        }

I think its reverting to checking index.js and not the folder. ^[a-z]+$ would make index.js pass but shouldn't let Bar pass.^a$ would make both fail.

When I run that test I now see:

1) lib/rules/filenames invalid export const Baz = {};:
     AssertionError: Should have 1 error but had 0: []
selaux commented 8 years ago

That is because Baz is not the default export, so the regex is used which passes. This looks like expected behavior to me.

mlucool commented 8 years ago

For index.js I thought the latest commit would look to the directory and just apply the rule there (as if it was a file). This way the folder than "looks like" a js file when importing and we can still follow the same naming conventions. Is this not the goal?

mlucool commented 8 years ago

@selaux any thoughts?

selaux commented 8 years ago

Sorry for the late answer, but I'm currently still undecided on how it should work. The problem I have with the current state is that some folders are linted (the ones that have an index.js file with a default export) but all others are not. This seems very inconsistent to me and would not be fixed by also linting folders with index.js files without default exports. Adding linting for all folder names would introduce other issues though, like finding the project root folder. Does anyone else here have an oppinion on that?

mlucool commented 8 years ago

In general folder naming linting is a positive - it forces style consistency just as much as file naming does. It would best be implemented by another optional specification of another regexp, which by defaults back to the file name one when not specified. Clearly this complicates things and I see why are you hesitant.

mlucool commented 8 years ago

Hi @selaux have you given this any more thought?

selaux commented 8 years ago

Actually I did. What do you think about splitting everything into two rules? One to match filenames to regexes and one to match file-/directorynames to exports. This would on one hand reduce the functionality, since the fallback mechanisms wouldn't be there anymore. On the other hand it would significantly simplify the rules and make them more transparent.

mlucool commented 8 years ago

I am happy with that idea sounds like it will confuse less people

selaux commented 8 years ago

Have a look at #8 this splits the rules.