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(manager/gradle): reimplement parser using tree-based approach #18663

Merged
merged 9 commits into from
Nov 15, 2022

Conversation

Churro
Copy link
Collaborator

@Churro Churro commented Oct 30, 2022

Changes

  • Re-implementation based on https://github.com/zharinov/good-enough-parser
  • Parsing capabilites are on par with the existing parser
  • Handler methods that process matches are functionally identical with the existing parser
    • For most parts of handler.ts, control and data flow were taken 1:1 from the existing implementation
    • Nesting was reduced, i.e., instead of doing if (dep) { ... } -> if (!dep) { return ctx; }
  • Tests were only adapted at points where unavoidable to demonstrate parity

Tokenizer, no longer needed types, etc of the existing parser are still there. I'd intend to do a follow-up cleaning PR.

apply from

The switch to the new parsing lib changed how apply from is implemented too:

  • Until now, files were read async and recursively passed to parseGradle (more context here)
  • The query interface of the new parser returns strictly Ctx, which means no async calls possible. I'm aware of 2 alternatives:

Context

#18484

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/modified unit tests, or
  • No unit tests but ran on a real repository, or
  • Both unit tests + ran on a real repository

@Churro Churro changed the title refactor(manager/gradle): reimplement parser using a tree-based approach refactor(manager/gradle): reimplement parser using tree-based approach Oct 30, 2022
@rarkins rarkins requested a review from zharinov October 31, 2022 04:13
zharinov
zharinov previously approved these changes Nov 10, 2022
@PhilipAbed
Copy link
Collaborator

looks good overall, will this PR solve #17657?

@rarkins rarkins changed the title refactor(manager/gradle): reimplement parser using tree-based approach feat(manager/gradle): reimplement parser using tree-based approach Nov 10, 2022
@rarkins rarkins requested a review from viceice November 10, 2022 19:09
…refactor

� Conflicts:
�	lib/modules/manager/gradle/parser.spec.ts
�	lib/modules/manager/gradle/parser.ts
@Churro
Copy link
Collaborator Author

Churro commented Nov 10, 2022

@PhilipAbed, this PR will not yet solve #17657 but a subsequent one. Implementation + tests for it are already ready and can be viewed in #18484

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.

otherwise LGTM

lib/modules/manager/gradle/parser/common.spec.ts Outdated Show resolved Hide resolved
lib/modules/manager/gradle/parser/common.ts Show resolved Hide resolved
lib/modules/manager/gradle/parser.ts Show resolved Hide resolved
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.

🤷‍♂️

@rarkins rarkins enabled auto-merge (squash) November 15, 2022 19:19
@rarkins rarkins merged commit b14336b into renovatebot:main Nov 15, 2022
@renovate-release
Copy link
Collaborator

🎉 This PR is included in version 34.26.0 🎉

The release is available on:

Your semantic-release bot 📦🚀

@github-actions github-actions bot locked as resolved and limited conversation to collaborators Dec 17, 2022
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.

6 participants