Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

feat(core): allow partial error text in log filters #3032

Merged
merged 11 commits into from
Jun 30, 2021
Merged

feat(core): allow partial error text in log filters #3032

merged 11 commits into from
Jun 30, 2021

Conversation

RaynalHugo
Copy link
Contributor

@RaynalHugo RaynalHugo commented Jun 16, 2021

Before all, let me hijack this PR to say that I've been using yarn for a few years now and I really like it. So thank you to all maintainers & contributors ❤️ Now is my turn to contribute.

What's the problem this PR addresses?

I haven't opened an issue about this and went directly with the PR. If you think that it would be better to actually discuss this in an issue, please tell me, I'll open one.

Here was my problem:

On a previous repository where we used yarn, we ended up having hundreds, if not thousands, of warnings during installs.
Recently, I've been working on a brand new repository where there is not a single warning at install. The maniac in me was really pleased. I wanted to keep it this way, so, I enforced it in the CI using the new logFilters feature. I added YN002 and YN060 warnings as errors to make sure that we wouldn't ignore them anymore.

However, there are some YN060 errors that I cannot easily solve. They need to update versions of really nested dependencies of dependencies. I thought I could use packageExtensions from yarnc.yml to "fix" it, but you cannot override an existing dependency version with this. I've also thought about resolutions, but I am not really comfortable with the side effects it can have.

So, I needed to set YN060 errors as warnings again.

I hoped that with logFilters I could use the text attribute to match only the ones that I can't fix to set them as errors, whereas leaving the rest as errors.

Something like:

logFilters:
  - code: YN060
    level: "error"
  - text:  "YN0060: MyPackage provides TheProblematicPackage"
    level: "warning"

After having a look at the code of logFilters, I realised that it wouldn't be possible. For the text key to match the message, it should be the entire error message and not a substring of the message

Closes #2603

How did you fix it?
I added the possibility to pass a RegExp or a substring as the text argument of a logFilter and check if it matches the error message.

Once again, I haven't opened an issue about that. Maybe it's not something you want for yarn or you have better solutions to propose. I am all ears.

Caveats
If there are multiple matches, the latest text-filter will be used. I don't know if this behaviour is wanted or not.

Checklist

  • I have set the packages that need to be released for my changes to be effective.
    I did but I am not sure that it should be a "minor" bump, maybe a "patch" one is enough.
  • I will check that all automated PR checks pass before the PR gets reviewed.

@RaynalHugo RaynalHugo requested a review from arcanis as a code owner June 16, 2021 17:26
@merceyz merceyz changed the title Enhancement/allow partial error text in log filters feat(core): allow partial error text in log filters Jun 16, 2021
Copy link
Member

@arcanis arcanis left a comment

Choose a reason for hiding this comment

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

Looks like a good start, thanks for tackling this! Two comments on the feature syntax:

  • I think it'd be best to use a different selector than text (for example pattern): since text is currently doing exact match, changing it would be a breaking change. While it's not impossible to do (especially since the next release will be major), I think there's a reasonable case to have an easy way to filter a message without having to care about special characters, and thus to keep text as it is.

  • Do we need regexps? Since we already use glob patterns in many places, I'd tend to prefer using a glob pattern here as well.

packages/yarnpkg-core/sources/formatUtils.ts Outdated Show resolved Hide resolved
@RaynalHugo
Copy link
Contributor Author

Thank you for looking into this.

I think the changes you proposed totally make sense, so:

  • I added the pattern keyword instead of reusing the text one.
  • I went for Glob patterns using micromatch (I saw it was already present in the repo so I guessed that is what you usually use).

There a still a few things that I would like to clarify with you regarding the API:

  • By using Globs instead of RegExp, we lose the ability to simply pass a substring.
    In other words pattern: "Lorem" won't match "Lorem Ipsum but pattern: "Lorem*" will. Would you like me to implement the required changes, or are we fine like this?
  • I gave the highest priority to the pattern-filters. I am not sure this is the best solution. Does this sound correct to you?

@arcanis
Copy link
Member

arcanis commented Jun 28, 2021

Perfect! I've just made a few changes to slightly improve perfs, and moved the pattern matching after the exact text matching (this way if you want to override all messages except one, you still can). I'll check if my changes keep the tests happy then it'll be good to merge 😃

@RaynalHugo
Copy link
Contributor Author

Nice!

moved the pattern matching after the exact text matching (this way if you want to override all messages except one, you still can)

Regarding this part, we may want to update the documentation accordingly.

@arcanis arcanis merged commit 2641319 into yarnpkg:master Jun 30, 2021
@olee
Copy link

olee commented Jul 8, 2021

Is this included in the latest release 2.4.2 already? (I saw it was updated 3 days ago which is after this was merged)

Because when I tried using this with the following configuration, I get an error telling me pattern is not supported:

logFilters:
  - code: YN0002
    level: discard
  - pattern: "* provides react * with version * which doesn't satisfy what *"
    level: discard
Usage Error: Unrecognized configuration settings found: logFilters[1].pattern - run "yarn config -v" to see the list of settings supported in Yarn in ...

@arcanis
Copy link
Member

arcanis commented Jul 8, 2021

No, new features exclusively reach the latest builds (ie the 3.x rcs, and from sources build). The patch release only receives critical patches, which so far have only been the TS patch upgrades.

@olee
Copy link

olee commented Jul 10, 2021

So how would I install those latest versions? Only with yarn set version from sources?

@merceyz
Copy link
Member

merceyz commented Jul 10, 2021

This will give you the latest RC (at the time of writing)

yarn set version 3.0.0-rc.9

@olee
Copy link

olee commented Sep 18, 2021

I tried using this with yarn 3.0.2 and the following config:

logFilters:
  - pattern: "* provides react *"
    level: discard

However, I still get this in the logs:

➤ YN0060: │ (...snip...) provides react (p6b185) with version 17.0.2, which doesn't satisfy what @craftjs/core and some of its descendants request
➤ YN0060: │ (...snip...) provides react (pf2e3b) with version 17.0.2, which doesn't satisfy what mobx-react-lite requests

@arcanis
Copy link
Member

arcanis commented Sep 18, 2021

Try replacing star by double star. It's glob, so slashes presumably mess with your pattern.

@merceyz
Copy link
Member

merceyz commented Sep 18, 2021

I tested that, doesn't help. Looking into it

@olee
Copy link

olee commented Sep 22, 2021

Same here - even using ** provides react ** which doesn't satisfy ** as a pattern still does not match a line like this:
@myScope/chat-frontend@workspace:packages/@myScope/chat-frontend provides react (pf2e3b) with version 17.0.2, which doesn't satisfy what mobx-react-lite requests

@merceyz
Copy link
Member

merceyz commented Sep 22, 2021

Should have been fixed in #3460, I just started a release build so you'll soon be able to run the following to get the fix

yarn set version berry && yarn set version canary

@olee
Copy link

olee commented Sep 23, 2021

Using the new version works properly, thank you.
But I'm still not sure about using micromatch which is for paths for the log patterns....

@arcanis
Copy link
Member

arcanis commented Sep 23, 2021

That's covered by micromatch/micromatch#219

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

[Feature] Add regex option to logFilters for partial matches
4 participants