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

fix(cake): quoted references #9692

Merged
merged 3 commits into from Apr 23, 2021
Merged

fix(cake): quoted references #9692

merged 3 commits into from Apr 23, 2021

Conversation

nils-a
Copy link
Contributor

@nils-a nils-a commented Apr 22, 2021

Changes:

I modified parser and lexer of the Cake manager to
recognize references that are fully set in quotes.

Context:

Closes #9691

Documentation (please check one with an [x])

  • I have updated the documentation, or
  • No documentation update is required

How I've tested my work (please tick one)

I have verified these changes via:

  • Code inspection only, or
  • Newly added unit tests, or
  • No new tests but ran on a real repository, or
  • Both unit tests + ran on a real repository

I modified parser and lexer of the Cake manager to
recognize references that are fully set in quotes.
@viceice viceice requested a review from zharinov April 23, 2021 03:52
Copy link
Member

@viceice viceice left a comment

Choose a reason for hiding this comment

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

LGTM, @zharinov can you check the lexer changes?

Copy link
Member

@viceice viceice left a comment

Choose a reason for hiding this comment

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

I think this should simplify it.

lib/manager/cake/index.ts Show resolved Hide resolved
lib/manager/cake/index.ts Outdated Show resolved Hide resolved
lib/manager/cake/index.ts Outdated Show resolved Hide resolved
@viceice
Copy link
Member

viceice commented Apr 23, 2021

Even if unrelated, please move const lexer to module scope for perfomance and memory reasons.

const lexer = moo.states(lexerStates);

@rarkins I think we (re)create too many moo lexers, which can be another memory and performance issue. So @zharinov can you open a pr to move those to module scope, so the lexers are created once?

@rarkins
Copy link
Collaborator

rarkins commented Apr 23, 2021

Does anything need to be changed for this PR to be accepted though?

@viceice
Copy link
Member

viceice commented Apr 23, 2021

Does anything need to be changed for this PR to be accepted though?

See my second review 😉

@zharinov
Copy link
Collaborator

zharinov commented Apr 23, 2021

@viceice I'll move all lexers to global scope in separate PR today

zharinov
zharinov previously approved these changes Apr 23, 2021
Copy link
Collaborator

@zharinov zharinov left a comment

Choose a reason for hiding this comment

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

LGTM

@viceice
Copy link
Member

viceice commented Apr 23, 2021

@zharinov What do you think about my suggestions to use a moo transform instead of triming the quotes later?

@zharinov zharinov self-requested a review April 23, 2021 06:16
use a moo transform instead of an additional if
@nils-a
Copy link
Contributor Author

nils-a commented Apr 23, 2021

@viceice I implemented the suggested change from if to a moo transform.

@nils-a nils-a requested a review from viceice April 23, 2021 06:55
@viceice viceice enabled auto-merge (squash) April 23, 2021 07:27
@viceice
Copy link
Member

viceice commented Apr 23, 2021

@rarkins Backport to v24?

@viceice viceice merged commit 37a8e28 into renovatebot:main Apr 23, 2021
@renovate-release
Copy link
Collaborator

🎉 This PR is included in version 25.2.1 🎉

The release is available on:

Your semantic-release bot 📦🚀

rarkins pushed a commit that referenced this pull request Apr 24, 2021
Co-authored-by: Michael Kriese <michael.kriese@visualon.de>
@github-actions github-actions bot locked as resolved and limited conversation to collaborators May 24, 2021
Sign up for free to subscribe to this conversation on GitHub. Already have an account? Sign in.
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

Cake referenced are ignored if they are in quotes
5 participants