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

feat: allow option for non-glob values for filename-blocklist #34

Conversation

AtlasTom
Copy link
Contributor

@AtlasTom AtlasTom commented Feb 8, 2024

This PR allows an new optional argument in the shema for the filename blocklist "stringResponse". This allows repos to deny a filenaming schema with an arbitrary reason (say a url for more info) rather than simply a strict globbing pattern.

new pattern:

{
        code: "var foo = 'bar';",
        filename: 'src/foo.models.ts',
        options: [
          {
            '*.models.ts': 'it is a generic naming convention. Please see http://someExample.com for our repo's rules and naming suggestions ',
          },
          true, //optional argument that allows the value string to not be checked as a globbing pattern
        ],
      },

response

$The filename "foo.models.ts" matches the blocklisted ".models.ts" pattern, this is not allowed because it is a generic naming convention. Please see http://someExample.com for our repo's rules and naming suggestions 

* @returns {boolean} true if reason is a string
* @param {any} reason an arbitrary reason for banning a file naming pattern that isn't a glob pattern
*/
export const stringResponseValidator = (reason) => typeof reason === 'string';
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 schema requires the value in the arguments to be a string, I'm not sure this is needed? However it seemed better than simply doing a ()=> true

Copy link
Owner

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I think () => true is ok.

Copy link
Owner

@dukeluo dukeluo left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Thanks for your PR. Could you also update the readme to describe the new param nonGlob?

stringResponse: {
type: 'boolean',
},
},
Copy link
Owner

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Would it be possible to make the new nonGlob parameter a property of an object, making it easier for future extensions?

{
  code: "var foo = 'bar';",
  filename: 'src/foo.models.ts',
  options: [
    {
      '*.models.ts': 'it is a generic naming convention. Please see http://someExample.com for our repo's rules and naming suggestions ',
    },
    {
      nonGlob: true //optional argument that allows the value string to not be checked as a globbing pattern
    }
  ],
}

You can refer this:

const { ignoreMiddleExtensions } = context.options[1] || {};

Copy link
Owner

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Maybe the variable name like nonGlobSuggestion will be better.

* @returns {boolean} true if reason is a string
* @param {any} reason an arbitrary reason for banning a file naming pattern that isn't a glob pattern
*/
export const stringResponseValidator = (reason) => typeof reason === 'string';
Copy link
Owner

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I think () => true is ok.


invalid: [],
}
);
Copy link
Owner

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Would it be possible to add another invalid test case and add the same test cases to the windows style test file filename-blocklist.windows.js?

@dukeluo dukeluo force-pushed the allow-arbitrary-string-reason-for-filename-blocklist branch from 7151ac1 to 84b6ac5 Compare February 14, 2024 04:00
@dukeluo dukeluo merged commit eb1836d into dukeluo:main Feb 14, 2024
3 checks passed
@dukeluo
Copy link
Owner

dukeluo commented Feb 17, 2024

@AtlasTom Since this new feature is similar with the feature describe in the issue #35, so I refactor the new param nonGli Suggestion as errorMessage. I think they can offer the same capabilities.

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.

2 participants