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

Changes

Addresses + documents the feature requests fromfeature idea: excluded_filenames #21 andexcluded_paths is Sort of Not Working #27 with a newexcluded_globs option. Note that this can technically be used instead ofexcluded_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 actionsdoes not support arrays
Added unit tests to test this change + a github action to run the tests
Replaced manual string concatenation withpath.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 aCONTRIBUTING.md is a good one for the long term.
Wasn't sure if the builtindex.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 triedminimatch,micromatch,picomatch, andnanomatch.nanomatch was the only one that worked with the leading./ (minimatch,micromatch), but it was last updated in 2018, isnot 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 supportingrelative 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)

hydrosquall added2 commits

August 14, 2021 17:00

chore(deps): add minimatch for pattern matching

0c8f385

chore(tooling): set up unit tests

2acdd5a

hydrosquall changed the title~~feature: add support for ignoring glob~~feature: add support for ignoring filepaths via glob

Aug 15, 2021

hydrosquall added2 commits

August 14, 2021 20:15

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

db1033a

…ixgithubocto#27)

chore(build): add CI to run unit tests

3c64a7e

hydrosquall force-pushed thecameron.yick/feature/ignore-files-and-folders branch from960f6fd to3c64a7eCompare

August 15, 2021 00:16

hydrosquall commented

Aug 15, 2021

View reviewed changes

src/process-dir.js

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

Copy link

ContributorAuthor

Choose a reason for hiding this comment

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

NamednodePath to avoid colliding with the existing usage ofpath as a variable name.

build(chore): update typechecker

fe02268

hydrosquall commented

Aug 15, 2021

View reviewed changes

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 tofolders toexclude from the diagram, separated by commas.

Copy link

ContributorAuthor

Choose a reason for hiding this comment

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

Technically this also worked for paths but only if they were immediate children ofroot_dir (this is whypackage.json worked, butsrc/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.

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",

Copy link

ContributorAuthor

hydrosquallAug 15, 2021

Choose a reason for hiding this comment

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

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

tsconfig.json

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

Copy link

ContributorAuthor

hydrosquallAug 15, 2021

Choose a reason for hiding this comment

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

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

hydrosquall commented

Aug 15, 2021

View reviewed changes

tsconfig.json Outdated


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

Copy link

ContributorAuthor

hydrosquallAug 15, 2021

Choose a reason for hiding this comment

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

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

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

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 ./

Copy link

Contributor

WattenbergerAug 16, 2021

Choose a reason for hiding this comment

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

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

Copy link

ContributorAuthor

hydrosquallAug 16, 2021

Choose a reason for hiding this comment

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

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.

Copy link

Contributor

Wattenberger commentedAug 16, 2021

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

Copy link

ContributorAuthor

hydrosquall commentedAug 16, 2021

this is wonderful@hydrosquall, thanks for digging in here! One question: what's your reasoning behind havingexcluded_pathsandexcluded_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 asnode_modules, you'd need to writenode_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 whatglobs are
having different filtering params than the web app is OK

repo-visualizer added8 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

src/process-dir.js Outdated

		const children = [];

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

Copy link

Contributor

WattenbergerAug 16, 2021

Choose a reason for hiding this comment

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

I thinkrootPath needs to bepath, to include the parents recursively?

Copy link

ContributorAuthor

hydrosquallAug 17, 2021

Choose a reason for hiding this comment

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

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

Copy link

Contributor

Wattenberger commentedAug 16, 2021

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/repo-visualizer@cameron.yick/feature/ignore-files-and-folders
(eg herehttps://github.com/Wattenberger/kumiko/blob/master/.github/workflows/diagram.yml#L14)

fix: specify paths relative to recursive instead of root

cd955d0

Copy link

ContributorAuthor

hydrosquall commentedAug 17, 2021

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

repo-visualizer/src/process-dir.js

Line 39 in7916b59

constinfo=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.

hydrosquall requested a review fromWattenberger

August 17, 2021 00:05

Copy link

Contributor

Wattenberger commentedAug 17, 2021

Wonderful, all looks great! Thanks!

Wattenberger merged commit0cd1a63 intogithubocto:main

Aug 17, 2021

hydrosquall deleted the cameron.yick/feature/ignore-files-and-folders branch

August 20, 2021 03:33

Labels

None yet

2 participants

Movatterモバイル変換

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

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

Uh oh!

Conversation

hydrosquall commentedAug 15, 2021• editedLoading Uh oh!There was an error while loading.Please reload this page.

Uh oh!

Motivation

Changes

Testing

Notes

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Wattenberger commentedAug 16, 2021

Uh oh!

hydrosquall commentedAug 16, 2021

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Wattenberger commentedAug 16, 2021

Uh oh!

hydrosquall commentedAug 17, 2021

Uh oh!

Wattenberger commentedAug 17, 2021

Uh oh!

Uh oh!

hydrosquall commentedAug 15, 2021•
edited
Loading