Skip to content

Conversation

@wedamija
Copy link
Member

When we match on path we currently try to access filename from the frame, and if not present we
use abs_path. This means that for any frames that have both filename and abs_path it's not
possible to match on a segment of the path.

This pr changes the behaviour of path to check both filename and abs_path. If we want to be
more conservative with this matching change we could add a new abs_path matching option, but I
feel like that would be adding more bloat to understanding this feature.

When we match on path we currently try to access `filename` from the frame, and if not present we
use `abs_path`. This means that for any frames that have both `filename` and `abs_path` it's not
possible to match on a segment of the path.

This pr changes the behaviour of `path` to check both `filename` and `abs_path`. If we want to be
more conservative with this matching change we could add a new `abs_path` matching option, but I
feel like that would be adding more bloat to understanding this feature.
@wedamija wedamija requested review from a team and untitaker October 28, 2021 02:11
@wedamija
Copy link
Member Author

@untitaker I pinged you here since you edited this recently, but not sure if you have any opinions on whether this makes sense.

Copy link
Member

@untitaker untitaker 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 is ok but I don't think the changes I made in ffaa6e8 qualify me to review this. I was just fixing a problem where frame.filename could be None

@wedamija wedamija merged commit 44786d6 into master Oct 28, 2021
@wedamija wedamija deleted the danf/project_owners_fix_path_match branch October 28, 2021 17:59
@github-actions github-actions bot locked and limited conversation to collaborators Nov 13, 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.

3 participants