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

Refactor object_usage_linter() to use xml_nodes_to_lint() #1285

Merged
merged 20 commits into from
May 26, 2022

Conversation

AshesITR
Copy link
Collaborator

@AshesITR AshesITR commented May 26, 2022

fixes #1284

  • remove get_function_assignments()
  • use more robust location matching via XPath
  • clean up (CC is 8 now)
  • rewrite get_content() to accept xml_node as info input and use line info as well
  • move usage parse adjustments to parse_check_usage()
  • improve get_assignment_symbols() by using get_r_string() instead of hand-rolled unescaping

@AshesITR AshesITR marked this pull request as ready for review May 26, 2022 13:25
@AshesITR AshesITR requested a review from MichaelChirico May 26, 2022 13:25
@AshesITR
Copy link
Collaborator Author

Need a news bullet for improved location info in #834 once merged.

Not happy with the performance atm. Looking into it.

@AshesITR
Copy link
Collaborator Author

Improvements halves the time, but still at 14.5s vs. 5.7s for main on tools/QC.R.
Willing to sacrifice some performance for the improved location info, but not that much.

@AshesITR
Copy link
Collaborator Author

Okay, turns out xml2::xml_find_num is way slower than xml2::xml_attr, so I wrote xp_find_location(xml, xpath) which special cases the two important cases number(./@col1) and number(./@col2).

This probably speeds up all linters using the default location info when they generate many lints.

@AshesITR AshesITR merged commit d0a9ce2 into main May 26, 2022
@AshesITR AshesITR deleted the node/object_usage branch May 26, 2022 20:17
@MichaelChirico MichaelChirico mentioned this pull request May 27, 2022
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.

Refactor object_usage_linter() to use xml_nodes_to_lint()
2 participants