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

Add new options for passing regexes to project root and strip path #398

Merged
merged 16 commits into from
May 1, 2020

Conversation

imjoehaines
Copy link
Member

@imjoehaines imjoehaines commented Apr 30, 2020

Goal

Allow using the bugsnag-php options for setting the project root and strip path as regexes, rather than just strings

This adds two new options, project_root_regex and strip_path_regex, which can be used instead of the existing project_root and strip_path options to allow setting them as regexes, as introduced in bugsnag/bugsnag-php#514

This is a continuation of #348

Changeset

Added

  • Two new configuration options:
    'project_root_regex' => env('BUGSNAG_PROJECT_ROOT_REGEX'),
    'strip_path_regex' => env('BUGSNAG_STRIP_PATH_REGEX'),
    These take presendence over the existing project_root and strip_path options if they are also provided
  • More tests for path resolution, building off of Add tests for "project root" and "strip path" resolution #395

Changed

  • Some of the existing path resolution code was leading to incorrect project root and strip path values being set, for example:
    • when a strip path value is configured, we only set the project root path if one wasn't provided. If both strip path and project root are given, no project root is set
    • when only strip path is configured, we set the strip path and then immediately overwrite it with a new path with "/app" appended by calling setProjectRoot.
      These issues have been fixed in this branch

Tests

More unit tests have been added, building off of #395

Discussion

Alternative Approaches

Outstanding Questions

Linked issues

Related to bugsnag/bugsnag-php#514
Related to #348

Review

For the submitter, initial self-review:

  • Commented on code changes inline explain the reasoning behind the approach
  • Reviewed the test cases added for completeness and possible points for discussion
  • A changelog entry was added for the goal of this pull request
  • Check the scope of the changeset - is everything in the diff required for the pull request?
  • This pull request is ready for:
    • Initial review of the intended approach, not yet feature complete
    • Structural review of the classes, functions, and properties modified
    • Final review

For the pull request reviewer(s), this changeset has been reviewed for:

  • Consistency across platforms for structures or concepts added or modified
  • Consistency between the changeset and the goal stated above
  • Internal consistency with the rest of the library - is there any overlap between existing interfaces and any which have been added?
  • Usage friction - is the proposed change in usage cumbersome or complicated?
  • Performance and complexity - are there any cases of unexpected O(n^3) when iterating, recursing, flat mapping, etc?
  • Concurrency concerns - if components are accessed asynchronously, what issues will arise
  • Thoroughness of added tests and any missing edge cases
  • Idiomatic use of the language

jpcid and others added 16 commits April 29, 2020 14:39
…oot should be configured first when strip is given
…t stripPath then it should go after project configuration
Regex > Paths > Default
Configure always project first to align with bugsnag-php which setup default strip path when setupProjectPath is invoked
Remove dead short execution neither strip or project is given, as placing default configuration at the beginning made that extra predetermined configurations no more necessary "in my opinion" ;)
This has some really horrible code that's far too complicated for
its own good, however all the tests pass so it should be easy
enough to refactor
Copy link
Contributor

@tomlongridge tomlongridge left a comment

Choose a reason for hiding this comment

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

LGTM 👍

@imjoehaines imjoehaines changed the base branch from test-path-resolution to next May 1, 2020 14:56
@imjoehaines imjoehaines merged commit f705a08 into next May 1, 2020
@imjoehaines imjoehaines deleted the setup-project-and-strip-regex branch May 1, 2020 14:57
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.

None yet

3 participants