fluid-project / fluid-lint-all

Consolidated linting logic free from any particular build technology
BSD 3-Clause "New" or "Revised" License
0 stars 5 forks source link

GH-1: Initial implementation (resolves #1). #2

Closed the-t-in-rtf closed 3 years ago

the-t-in-rtf commented 3 years ago

Initial implementation of gruntless linting. Addresses #1.

the-t-in-rtf commented 3 years ago

This is ready for initial review, but there a few things I already know I want to talk through together:

  1. The markdownlint checks do not work correctly with the line-length rules, and I haven't been able to figure out why yet. Using the equivalent canned settings works.
  2. I'm not happy with the implicit merging of includes/excludes, as it does nothing to mitigate the issues with fluid.extend and arrays.
  3. I could use help figuring out how to inherit material, say using the same includes for all JSON checks. I was thinking of making a single lightweight config holder grade that expands/merges the canned options.
the-t-in-rtf commented 3 years ago

I found a fix for the line-length rules, although I'm a bit baffled. If you look at my last commit, all I did was change from using fluid.extend to fluid.copy.

the-t-in-rtf commented 3 years ago

@amb26, I am happier with the support for inheritance and options merging now.

We didn't quite manage to merge the pull to use a standard markdownlint configuration package before I started writing this, I updated the work here to do that as well.

the-t-in-rtf commented 3 years ago

I also figured out the CI config, I had copied an older config with the outdated branch name instead of main.

the-t-in-rtf commented 3 years ago

@jobara, this is far enough along that you can try it on any package. There's no published (non-dev) release, and the first release doesn't work with npx, so you have to use the latest dev release manually for now, as in:

cd your-project
npx fluid-lint-all@1.0.0-dev.20201222T140010Z.b32ae9c.GH-1
the-t-in-rtf commented 3 years ago

@greatislander, this now also includes the standard markdownlint config. You can see the effective options by passing the (non-API) showMergedConfig flag, as in:

npx fluid-lint-all@1.0.0-dev.20201222T140010Z.b32ae9c.GH-1 --showMergedConfig

greatislander commented 3 years ago

@the-t-in-rtf Thanks! I'll try this out.

Not related to this code specifically, but one thing we might want to consider for pull requests which address Github issues as opposed to JIRAs is to use the Github convention to automatically close the issue when the pull request is merged. For more information, see "Linking a pull request to an issue" (in this case one might change the PR title to Initial implementation (resolves #1). Is this worth documenting on the Wiki somewhere?

EDIT: I see that the GH-<issue> prefix automatically links to issues but I'm not sure if it will trigger closing the issue on merge without one of those keywords.

the-t-in-rtf commented 3 years ago

I tested the npx functionality extensively using a package that doesn't already use our lint checks (flocking).

Initially I had the same kind of worrying plugin dependency resolution issues we saw with Grunt (we use the jsdocs plugin and the markdown plugin).

Thankfully ESLint provides a key option to configure where to look for plugins, and after updating all ESLint checks to use this, it seems to address the issues I was seeing.

the-t-in-rtf commented 3 years ago

Last failure was a linting error. I added the package's linting to my precommit hooks.

the-t-in-rtf commented 3 years ago

Quick summary, I still need to:

  1. Address your comments about error handling.
  2. Write some JSDocs
  3. Exercise the updated code in other projects.
the-t-in-rtf commented 3 years ago

I believe this is ready for further review. I will test the package in downstream projects shortly, I cut 1.0.0-dev.20210108T112637Z.3a60d59.GH-1 for that purpose.

the-t-in-rtf commented 3 years ago
```shell
12:22:25.936:    - src/assets/images/image8.png:
12:22:25.936:      (194:-) Expected a newline at the end of the file.

I think if the config file needs to specify file extensions for this check, the README should probably indicate that.

That check has to be pretty broad to avoid missing text files with no extension and the huge range of extensions that text files might have. I'lll add a note as you suggest, that seems like a good way forward. Note that you'd probably just exclude src/assets/images in your example, you don't necessarily have to get down to excluding individual extensions.

the-t-in-rtf commented 3 years ago

The lintspaces.newlines check is run against image files when no includes/excludes configuration is provided

I added an additional example with a clear explanation. I also double-checked the node-lintspaces documentation to confirm that they don't provide a means of excluding binary files.

amb26 commented 3 years ago

I think the portion of the tests which performs self-linting should be packaged as a standard linting task (if nothing else, as an example) so that "npm run lint" will, as is usual, lint this package itself.

the-t-in-rtf commented 3 years ago

Seems fair, and done.