feature: add support for ignoring filepaths via micromatch globs #28

hydrosquall · 2021-08-15T00:11:20Z

Motivation

Give users the necessary controls to make readable code fingerprints through a widely used pattern matching format, the minimatch / micromatch glob. You can debug/test the format here
Continuing the work started w/ @Wattenberger in https://github.com/hydrosquall/untitled-code-map :)

Changes

Addresses + documents the feature requests from feature idea: excluded_filenames #21 and excluded_paths is Sort of Not Working #27 with a new excluded_globs option. Note that this can technically be used instead of excluded_paths, but I didn't want to break anyone's existing workflow by modifying an existing option.
- Uses ; as a delimiter since , can show up in globs, and because github actions does not support arrays
Added unit tests to test this change + a github action to run the tests
Replaced manual string concatenation with path.join() so that this will work on Windows

Testing

I was leaning on unit tests, I haven't run this set of changes personally (not sure how to test a GH action locally). @swharden's idea to add a CONTRIBUTING.md is a good one for the long term.
Wasn't sure if the built index.js was supposed to get checked into the repo, or gitignored, but recent PRs seem to be updating it so I left it as-is.

Notes

I tried minimatch, micromatch, picomatch, and nanomatch. nanomatch was the only one that worked with the leading ./ (minimatch, micromatch), but it was last updated in 2018, is not as performant, and is not as full featured as micromatch.
I think micromatch syntax is probably a safe bet given its widespread usage (webpack, babel, eslint, etc).
There's not much incentive to be supporting relative paths, since repositories shouldn't be ever trying to access parent directories, I think we'd rather use a more popular globbing library than use
During the brief period where nanomatch seemed like a good option, I came up with a TS file: Typecript typings micromatch/nanomatch#21 (comment)

…ix githubocto#27)

hydrosquall · 2021-08-15T00:18:49Z

src/process-dir.js

@@ -1,15 +1,18 @@
 import fs from "fs";
+import * as nodePath from 'path';


Named nodePath to avoid colliding with the existing usage of path as a variable name.

hydrosquall · 2021-08-15T00:21:16Z

README.md

@@ -18,19 +18,33 @@ Default: diagram.svg

 ## `excluded_paths`

-A list of paths to exclude from the diagram, separated by commas.
+A list of paths to folders to exclude from the diagram, separated by commas.


Technically this also worked for paths but only if they were immediate children of root_dir (this is why package.json worked, but src/package.json did not. Rather than explain this nuance, I think it'll be simpler to just only use this config for folders, and nudge people towards mostly using globs instead.

hydrosquall · 2021-08-15T00:21:57Z

package.json

  },
  "dependencies": {
    "@actions/core": "^1.4.0",
    "@actions/exec": "^1.1.0",
    "d3": "^7.0.0",
    "esbuild": "^0.12.15",
-    "husky": "^7.0.1",


I figured it was probably a typo to have 2 versions of the same library.

hydrosquall · 2021-08-15T00:34:18Z

tsconfig.json

@@ -0,0 +1,19 @@
+{
+  "compilerOptions": {


This is trimmed down from the defaults provided by tsc --init, and was included for the benefit of the test-runner.

hydrosquall · 2021-08-15T00:35:16Z

tsconfig.json


-    /* Strict Type-Checking Options */
-    "strict": true,                                 /* Enable all strict type-checking options. */


This was too strict, having all of these on was catching 100s of errors.

Wattenberger · 2021-08-16T13:56:00Z

src/index.jsx

@@ -22,14 +22,19 @@ const main = async () => {
  core.endGroup()


-  const rootPath = core.getInput("root_path") || "./";
+  const rootPath = core.getInput("root_path") || ""; // Micro and minimatch do not support paths starting with ./


we might want to also remove any leading ./ params, to prevent from breaking existing workflows

I thought about this - what do you think about a choice between

Quitting the script early if the path starts with ./ and asking the user to remove it

Quietly removing it if it's detected

I think doing 2 automatically is a smoother migration because it introduces a quiet side effect, but 1 is simpler for us to maintain. Otherwise, people may get confused when seeing some configs start with ./ and some without.

Wattenberger · 2021-08-16T13:58:44Z

this is wonderful @hydrosquall, thanks for digging in here! One question: what's your reasoning behind having excluded_paths and excluded_globs as separate params? Is it possible to have one combined param?

hydrosquall · 2021-08-16T16:11:01Z

this is wonderful @hydrosquall, thanks for digging in here! One question: what's your reasoning behind having excluded_paths and excluded_globs as separate params? Is it possible to have one combined param?

Hi, thank you! I had a few reasons for going with this

Backwards compatibility- it was safer to add a new param with new behavior than to modify an existing param.
excluded_paths is slightly terser/beginner friendly: to express the current filters as node_modules, you'd need to write node_modules/**. It's also slightly more performant to do an excluded_path check (testing set membership vs compiling and running a regular expression), but this is probably a negligible cost for most cases.
excluded_paths matches the behavior of the web app file filters, in case people are copy pasting their exclusion filters from there (as far as I can tell, that's a separate codebase, otherwise I would have updated the matching logic in both spots).

With all of that said, I would be in favor of using the glob param, assuming that

it's OK to drop the current param (maybe we leave the old one in as a no-op, and throw an error or warning if people try to set it)
our target users are familiar with the idea of what globs are
having different filtering params than the web app is OK

Wattenberger · 2021-08-16T18:43:01Z

src/process-dir.js

+        const children = [];
+
+        for (const fileOrFolder of filesOrFolders) {
+          const fullPath = nodePath.join(rootPath, fileOrFolder);


I think rootPath needs to be path, to include the parents recursively?

You're absolutely right, that was a bug :)

Wattenberger · 2021-08-16T18:45:49Z

I did some testing and made some small tweaks when running into bugs. It works, but ignores folders - I'm thinking because of the comment above.

FYI you can test the Action by pointing it at any repo@branch:
uses: hydrosquall/[email protected]/feature/ignore-files-and-folders
(eg here https://github.com/Wattenberger/kumiko/blob/master/.github/workflows/diagram.yml#L14)

hydrosquall · 2021-08-17T00:05:50Z

Thanks for fixing the yaml / ./ prefix errors - they look good. I made the adjustment to the fullPath calculation, and am optimistic this should fix it since it restores the previous behavior at

repo-visualizer/src/process-dir.js

Line 39 in 7916b59

const info = fs.statSync(path + "/" + file);

.

Also, thanks for the tip about how to test github actions - I will use this to try out dev builds going forward.

Wattenberger · 2021-08-17T13:43:54Z

Wonderful, all looks great! Thanks!

hydrosquall added 2 commits August 14, 2021 17:00

chore(deps): add minimatch for pattern matching

Verified

This commit was signed with the committer’s verified signature.

Verified
Learn about vigilant mode

0c8f385

chore(tooling): set up unit tests

Verified

This commit was signed with the committer’s verified signature.

Verified
Learn about vigilant mode

2acdd5a

hydrosquall changed the title ~~feature: add support for ignoring glob~~ feature: add support for ignoring filepaths via glob Aug 15, 2021

hydrosquall added 2 commits August 14, 2021 20:15

feat: enabling filtering via excluded_globs param (Fix githubocto#21, F…

Verified

This commit was signed with the committer’s verified signature.

Verified
Learn about vigilant mode

db1033a

…ix githubocto#27)

chore(build): add CI to run unit tests

3c64a7e

hydrosquall force-pushed the cameron.yick/feature/ignore-files-and-folders branch from 960f6fd to 3c64a7e Compare August 15, 2021 00:16

hydrosquall commented Aug 15, 2021

View reviewed changes

build(chore): update typechecker

fe02268

hydrosquall commented Aug 15, 2021

View reviewed changes

hydrosquall changed the title ~~feature: add support for ignoring filepaths via glob~~ feature: add support for ignoring filepaths via micromatch globs Aug 15, 2021

docs: update parameters in github action config for exclude_globs

3343a6a

Wattenberger reviewed Aug 16, 2021

View reviewed changes

repo-visualizer added 8 commits August 16, 2021 14:09

fix syntax error in action.yml

8b4ad8b

don't require root_path

86ecc39

default path to ./ for fs methods

264b40d

dont check root path for glob or path match

cf2f11b

dont check root path for glob or path match

986fc25

dont exclude paths on empty glob

35d3bd0

ensure paths are relative in fs methods

e30bbb1

ensure paths are relative in fs.statSync

6255064

Wattenberger reviewed Aug 16, 2021

View reviewed changes

fix: specify paths relative to recursive instead of root

cd955d0

hydrosquall requested a review from Wattenberger August 17, 2021 00:05

Wattenberger merged commit 0cd1a63 into githubocto:main Aug 17, 2021

hydrosquall deleted the cameron.yick/feature/ignore-files-and-folders branch August 20, 2021 03:33

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

feature: add support for ignoring filepaths via micromatch globs #28

feature: add support for ignoring filepaths via micromatch globs #28

hydrosquall commented Aug 15, 2021 •

edited

Loading

hydrosquall Aug 15, 2021

hydrosquall Aug 15, 2021

hydrosquall Aug 15, 2021

hydrosquall Aug 15, 2021

hydrosquall Aug 15, 2021

Wattenberger Aug 16, 2021

hydrosquall Aug 16, 2021

Wattenberger commented Aug 16, 2021

hydrosquall commented Aug 16, 2021

Wattenberger Aug 16, 2021

hydrosquall Aug 17, 2021

Wattenberger commented Aug 16, 2021

hydrosquall commented Aug 17, 2021

Wattenberger commented Aug 17, 2021

		@@ -1,15 +1,18 @@
		import fs from "fs";
		import * as nodePath from 'path';


		/* Strict Type-Checking Options */
		"strict": true, /* Enable all strict type-checking options. */

feature: add support for ignoring filepaths via micromatch globs #28

feature: add support for ignoring filepaths via micromatch globs #28

Conversation

hydrosquall commented Aug 15, 2021 • edited Loading

Motivation

Changes

Testing

Notes

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Wattenberger commented Aug 16, 2021

hydrosquall commented Aug 16, 2021

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Wattenberger commented Aug 16, 2021

hydrosquall commented Aug 17, 2021

Wattenberger commented Aug 17, 2021

hydrosquall commented Aug 15, 2021 •

edited

Loading