Feature proposal: provide function to filter #1050

Primajin · 2022-02-11T15:45:04Z

While thinking about how to solve #1034 I was playing around a bit and came up with this draft.

The idea is that one can provide a function - for example via the .ncurc.js

target: (s) => { return s === matchesMyComplicatedChecksHere }

and then only get's back the matching packages. An alternative to a fancy regex where it can be solved more in detail and programmatically.

test provided:

/*dependencies: {
  mocha: '1.2'
  lodash: '^3.9.3'
  moment: '^1.0.0'
  chalk: '^1.1.0'
  bluebird: '^1.0.0'
}*/
    
it('filter dependencies by function', () => {
  getCurrentDependencies(deps, { filter: (s:string) => s.startsWith('m') }).should.eql({
    mocha: '1.2',
    moment: '^1.0.0'
  })
})

What do you think?

raineorshine

Looks great!

Update cli-options.
Let's add tests for reject, filterVersion, and rejectVersion as well. I think they use the same predicate so they automatically get support for filter functions with this PR.
Pass name and versionRange to the predicate function as described here: [FR] Introduce safe option #1034 (comment). This will make it easier for developers to write conditions using the dependency version. In effect, filter and filterVersion will be identical when a function is passed. I think it's simpler than having two separate predicate signatures.

Primajin · 2022-02-17T12:44:42Z

Hello there 👋🏻 😄
I am currently adding the tests for reject, filterVersion, and rejectVersion:

I found that test/getCurrentDependencies.test.ts has tests for filter and reject - but the tests for filterVersion and rejectVersion look very differently and are inside test/index.test.ts.
Are you ok if I add a couple more tests to test/getCurrentDependencies.test.ts in the same style as filter and reject but for filterVersion and rejectVersion?

Then I found that filter and filterVersion accept

filterVersion?: string | string[] | RegExp | RegExp[],

while reject and rejectVersion only accept:

rejectVersion?: string | string[] | RegExp,

Is that deliberate? If not, are you OK if I type them all with FilterRejectPattern ?
Does the JSDoc need to be updated as well?

Speaking of JSDoc - I'm not a native speaker so up to you to decide, but my IDE was nagging me about a missing the - shall I add it while at it?

Primajin · 2022-02-17T12:53:44Z

Oh apparently the CLI options are also contradicting with the types:

  {
    long: 'filter',
    short: 'f',
    arg: 'matches',
    description: 'Include only package names matching the given string, wildcard, glob, comma-or-space-delimited list, or /regex/.',
    type: 'string | string[] | RegExp',
  },
  {
    long: 'filterVersion',
    arg: 'matches',
    description: 'Filter on package version using comma-or-space-delimited list, or /regex/.',
    type: 'string | string[] | RegExp',
  },
  {
    long: 'reject',
    short: 'x',
    arg: 'matches',
    description: 'Exclude packages matching the given string, wildcard, glob, comma-or-space-delimited list, or /regex/.',
    type: 'string | string[] | RegExp | RegExp[]',
  },
  {
    long: 'rejectVersion',
    arg: 'matches',
    description: 'Exclude package.json versions using comma-or-space-delimited list, or /regex/.',
    type: 'string | string[] | RegExp | RegExp[]',
  },

vs.

  /**
   * Include only package names matching the given string, wildcard, glob, comma-or-space-delimited list, or /regex/.
   */
  filter?: string | string[] | RegExp | RegExp[],

  /**
   * Filter on package version using comma-or-space-delimited list, or /regex/.
   */
  filterVersion?: string | string[] | RegExp | RegExp[],

  /**
   * Exclude packages matching the given string, wildcard, glob, comma-or-space-delimited list, or /regex/.
   */
  reject?: string | string[] | RegExp,

  /**
   * Exclude package.json versions using comma-or-space-delimited list, or /regex/.
   */
  rejectVersion?: string | string[] | RegExp,

So they are exactly the opposite.

I would make them all string | string[] | RegExp | RegExp[] | Function now - ok?

raineorshine · 2022-02-17T13:19:42Z

I found that test/getCurrentDependencies.test.ts has tests for filter and reject - but the tests for filterVersion and rejectVersion look very differently and are inside test/index.test.ts.
Are you ok if I add a couple more tests to test/getCurrentDependencies.test.ts in the same style as filter and reject but for filterVersion and rejectVersion?

Sure!

Is that deliberate? If not, are you OK if I type them all with FilterRejectPattern ?
Does the JSDoc need to be updated as well?

It is not deliberate. I think they should have the same type. Thanks!

Speaking of JSDoc - I'm not a native speaker so up to you to decide, but my IDE was nagging me about a missing the - shall I add it while at it?

You are right! That code was written by a non-native speak also, but your IDE apparently is a native speaker :).

I would make them all string | string[] | RegExp | RegExp[] | Function now - ok?

Yes, I agree.

Primajin · 2022-02-17T14:09:52Z

Update cli-options.

Let's add tests for reject, filterVersion, and rejectVersion as well. I think they use the same predicate so they automatically get support for filter functions with this PR.

done 😄

Pass name and versionRange to the predicate function as described here: #1034 (comment). This will make it easier for developers to write conditions using the dependency version. In effect, filter and filterVersion will be identical when a function is passed. I think it's simpler than having two separate predicate signatures.

Should this be part of this MR or a separate one - sunsetting filterVersion/rejectVersion feels like a breaking change that would require a major version bump - while the rest could be a new feature with a minor bump so far.
What do you think?

raineorshine · 2022-02-17T14:39:19Z

Pass name and versionRange to the predicate function as described here: #1034 (comment). This will make it easier for developers to write conditions using the dependency version. In effect, filter and filterVersion will be identical when a function is passed. I think it's simpler than having two separate predicate signatures.

Should this be part of this MR or a separate one - sunsetting filterVersion/rejectVersion feels like a breaking change that would require a major version bump - while the rest could be a new feature with a minor bump so far. What do you think?

It should be part of this PR, since I don't want to publish the feature with the wrong predicate signature.

My intention was to keep filterVersion and rejectVersion, so this won't be a breaking change. It's just the function signature of the predicate function that is in question. You can pass the result of semverutils.parseRange(versionRange) as the second argument to the predicate, so that the developer can write conditions based on the version number.

Primajin · 2022-02-17T15:02:40Z

🤔
Did you mean like this: Primajin@d21ec5a

Given:

 @types/chai-as-promised             ^7.1.4  →    ^7.1.5
 @types/node                       ^17.0.10  →  ^17.0.18
 @typescript-eslint/eslint-plugin   ^5.10.0  →   ^5.12.0
 @typescript-eslint/parser          ^5.10.0  →   ^5.12.0

.ncurc.js

filter: (name,version) => {return name.startsWith('@types') && version.startsWith('^7')},

yields:

Sorry if I can't fully follow.

raineorshine · 2022-02-17T15:27:46Z

Yes, but version should be of type semverutils.SemVer[]. We parse the version range for the developer to make it even easier to write conditions.

You can see semverutils.parseRange used throughout the codebase.

See: https://git.coolaj86.com/coolaj86/semver-utils.js#semverutils-parserange-rangestring

Primajin · 2022-02-17T16:07:14Z

Sorry but I can't follow / understand how to implement that.
I am getting the current version as string from the package.json and then how should it be parsed into a range and decided as "this returns true now"?
Signing off for today, thanks for the comments and help though 🙂 👍🏻

raineorshine · 2022-02-18T15:52:25Z

No problem. I'll revisit when I have a moment.

raineorshine · 2022-02-18T22:26:39Z

This is what I had in mind: 13cafc5

The filterVersion predicate function signature doesn't match filter, but I'm going to leave it for now since the composeFilter pipeline needs major refactoring to support this.

It looks ready to merge from my perspective. Let me know if I'm missing anything.

Primajin · 2022-02-19T15:25:14Z

Ahh cool thanks! 👍🏻 Let me take a final glimpse at it on Monday - thanks for your support!

Primajin · 2022-02-19T15:41:59Z

OK couldn't wait until Monday 😅

Added a final cleanup commit with tiny changes:

src/lib/filterAndReject.ts:4 - we don't need to import all of semverutils here, we can just get the two things we need
src/types.ts:28 - FilterFunction isn't used outside any more, no need to export
test/getCurrentDependencies.test.ts - both of us added dangling commas, while I personally like them, it looks like the rest of the codebase doesn't use them - so let's adhere to the common style 😉

Very excited about this, it was a lot of fun - thank you!

raineorshine · 2022-02-20T13:43:49Z

Thanks!

test/getCurrentDependencies.test.ts - both of us added dangling commas, while I personally like them, it looks like the rest of the codebase doesn't use them - so let's adhere to the common style 😉

I need to add Prettier...

raineorshine · 2022-02-20T13:47:01Z

Published to v12.4.0

Note that I am going to patch the filterVersion predicate when I have time to do the necessary refactoring. It should have the same signature as the filter predicate. I'll update the README and publish an official release then.

Primajin added 3 commits February 11, 2022 15:58

fix jsdoc

631d075

allow to provide a function as filter

3873911

add test

dc6451a

Primajin mentioned this pull request Feb 11, 2022

[FR] Introduce safe option #1034

Closed

raineorshine requested changes Feb 11, 2022

View reviewed changes

Primajin added 2 commits February 17, 2022 13:48

add test for reject by function

1e82454

update cli options

a83e4ea

Primajin added 3 commits February 17, 2022 14:35

move dep tests into dep describe

f548794

add tests for filterVersion and rejectVersion

c6780e4

change grammar of doc

7719430

Primajin added 3 commits February 17, 2022 16:40

pass dependency and version

bcede72

type version as SemVer

eff519d

make typing consistent

39e7509

Pass SemVer[] to filter predicate.

13cafc5

nit: final cleanup

f71f2ab

Primajin marked this pull request as ready for review February 19, 2022 15:43

Merge branch 'main' into override-filter

1199792

Primajin requested a review from raineorshine February 19, 2022 15:44

raineorshine approved these changes Feb 20, 2022

View reviewed changes

raineorshine merged commit 087ab09 into raineorshine:main Feb 20, 2022

Primajin deleted the override-filter branch February 20, 2022 14:03

Primajin mentioned this pull request Feb 28, 2022

Target as a function #1052

Merged

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

Feature proposal: provide function to filter #1050

Feature proposal: provide function to filter #1050

Primajin commented Feb 11, 2022

raineorshine left a comment •

edited

Primajin commented Feb 17, 2022 •

edited

Primajin commented Feb 17, 2022

raineorshine commented Feb 17, 2022

Primajin commented Feb 17, 2022 •

edited

raineorshine commented Feb 17, 2022 •

edited

Primajin commented Feb 17, 2022

raineorshine commented Feb 17, 2022

Primajin commented Feb 17, 2022

raineorshine commented Feb 18, 2022

raineorshine commented Feb 18, 2022

Primajin commented Feb 19, 2022

Primajin commented Feb 19, 2022

raineorshine commented Feb 20, 2022

raineorshine commented Feb 20, 2022

Feature proposal: provide function to filter #1050

Feature proposal: provide function to filter #1050

Conversation

Primajin commented Feb 11, 2022

raineorshine left a comment • edited

Choose a reason for hiding this comment

Primajin commented Feb 17, 2022 • edited

Primajin commented Feb 17, 2022

raineorshine commented Feb 17, 2022

Primajin commented Feb 17, 2022 • edited

raineorshine commented Feb 17, 2022 • edited

Primajin commented Feb 17, 2022

raineorshine commented Feb 17, 2022

Primajin commented Feb 17, 2022

raineorshine commented Feb 18, 2022

raineorshine commented Feb 18, 2022

Primajin commented Feb 19, 2022

Primajin commented Feb 19, 2022

raineorshine commented Feb 20, 2022

raineorshine commented Feb 20, 2022

raineorshine left a comment •

edited

Primajin commented Feb 17, 2022 •

edited

Primajin commented Feb 17, 2022 •

edited

raineorshine commented Feb 17, 2022 •

edited