mmkal / pgkit

PostgreSQL🤝TypeScript monorepo. SQL client/admin UI/smart migrator/type generator/schema inspector
https://pgkit.dev
190 stars 25 forks source link

change(cli): harmonise glob/ignore/since behaviour #382

Closed janpaepke closed 2 years ago

janpaepke commented 2 years ago

Spoiler: This is a breaking change, as options changed and were added.

As you know I stumbled across some type issues recently, when updating the dependencies. This lead me down a rabbit hole to restructure the way the script deals with match/ignore and since.

The main issues were:

  1. Ignore was not configurable via CLI
  2. Both the glob pattern as well as the ignore pattern were discarded, when using since.

The second one is what actually drove me to address this. You couldn't configure both since and glob/ignore in the config at the same time. Even more surprisingly, if you configured glob or ignore patterns using the config, they would be discarded, when using since in CLI. In since mode it would process all modified files, even if they don't match the glob pattern. This was particularly pointless in watch mode, where since doesn't even have any use. All this was fairly unexpected behaviour to a consumer.

I solved this by adding ignore and since as an extra option (also as a CLI option in the case of ignore) and made them behave as one would expect. This also simplifies configuring the glob pattern, which is now independent from the other two. I also made it more transparent in the logs which matching criteria actually applies.

Personally I would rename glob to match and now might be the time to do it, as this is breaking anyway, but this is highly subjective, so I left it as it is.

Up next: Type Intersections, as discussed via email.

codecov-commenter commented 2 years ago

Codecov Report

Merging #382 (0592f0b) into master (3d94770) will increase coverage by 0.00%. The diff coverage is 100.00%.

@@           Coverage Diff           @@
##           master     #382   +/-   ##
=======================================
  Coverage   96.66%   96.66%           
=======================================
  Files          32       32           
  Lines        1110     1111    +1     
  Branches      210      210           
=======================================
+ Hits         1073     1074    +1     
  Misses         37       37           
Impacted Files Coverage Δ
packages/typegen/src/cli.ts 100.00% <100.00%> (ø)
packages/typegen/src/defaults.ts 100.00% <100.00%> (ø)
packages/typegen/src/index.ts 99.31% <100.00%> (ø)
packages/typegen/src/util.ts 100.00% <100.00%> (ø)

Continue to review full report at Codecov.

Legend - Click here to learn more Δ = absolute <relative> (impact), ø = not affected, ? = missing data Powered by Codecov. Last update 3d94770...0592f0b. Read the comment docs.

janpaepke commented 2 years ago

no need to apologise, I find our "back and forth" to be quite productive.

exclude actually already is an array (string or array), because that is what ignore is in the glob options. I was actually thinking about making glob (now include) the same type the initial pr, but kept it a pure string, because it is in glob (Which was strange to me - why would they make the match pattern a string and the ignore option a string or array of strings?).

But since we now abandoned glob adherence, your suggestion does make sense. To confirm: you want to make both include and exclude only string[], right?

mmkal commented 2 years ago

Yes I think so - mostly to match tsconfig. I’d also rather only allow string just to keep things simple.

janpaepke commented 2 years ago

You're giving me mixed signals here 😆 I'll just change them to string[]

  1. because in CLI it's still exactly the same, with the difference that you can add multiple --include options
  2. We'll avoid having to explain to people how glob works, when they raise issues asking how to add multiple patterns.
janpaepke commented 2 years ago

Or did you mean to write

I’d also rather only allow array of string just to keep things simple.

as in "only string[] not string | string[]"

Because then: yes.

mmkal commented 2 years ago

Or did you mean to write

I’d also rather only allow array of string just to keep things simple.

as in "only string[] not string | string[]"

Because then: yes.

🤦yes that’s what I meant - v unhelpful typo!

janpaepke commented 2 years ago

should be good now. Hope I didn't overlook some docs somewhere. 🙈