Skip to content

fix(remappings): properly filter only autodetected remappings, do not sort unnecesarily#5562

Merged
mattsse merged 7 commits intomasterfrom
evalir/fix-remapping-filter
Aug 8, 2023
Merged

fix(remappings): properly filter only autodetected remappings, do not sort unnecesarily#5562
mattsse merged 7 commits intomasterfrom
evalir/fix-remapping-filter

Conversation

@Evalir
Copy link
Member

@Evalir Evalir commented Aug 8, 2023

Motivation

Closes #5561.

The issue highlighted a few edge cases:

  • we should filter with starts_with, rather than contains
  • we should only filter autodetected remappings, instead of all remappings (as it might be intended for the user to have several keys pointing to the same location)
  • we should filter without sorting, so filter instead of deduping.

Solution

  • Adds an into_inner method to the wrapper which produces a vector of remappings that are unique.
  • removes unwanted sorting/filtering

@Evalir Evalir requested a review from mattsse August 8, 2023 14:11
Copy link
Member

@mattsse mattsse left a comment

Choose a reason for hiding this comment

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

few nits,

would like an integration test for forge remappings with remappings.txt

@Evalir Evalir force-pushed the evalir/fix-remapping-filter branch from b94e9f1 to 40c5317 Compare August 8, 2023 15:40
@Evalir Evalir force-pushed the evalir/fix-remapping-filter branch from 40c5317 to fdefce3 Compare August 8, 2023 15:43
@Evalir Evalir requested a review from mattsse August 8, 2023 15:50
@mattsse mattsse merged commit 1a110a5 into master Aug 8, 2023
@mattsse mattsse deleted the evalir/fix-remapping-filter branch August 8, 2023 16:10
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.

fix(remappings): proposing rollback changes that deduplicates insertion of remappings entry with .contains() method

2 participants