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

User defined mapping: add option and handling for multiple mapping files #306

Merged

Conversation

mknorps
Copy link
Collaborator

@mknorps mknorps commented Apr 14, 2023

Extend --custom-mapping-file option to handle more than one mapping file.

With this change, improved handling of repeated package entry in custom mapping is introduced. We use normalized package names (as they are used only for debug messages) and prepare a CustomMapping dictionary from all sources of the custom mapping, be it files or entries from falwtydeps config.

Commits may be read one by one:

  • 9cb1629 introduces the cli option and do not change behavior of current implementation
  • 44011a0 adds actual handling of multiple files
  • 44011a0 adds tests for combination of different mapping inputs
  • 0a0546a better handling of multiple packages mapping in input files (following a team meeting conversation with @Nour-Mws and @jherland)

@mknorps mknorps linked an issue Apr 14, 2023 that may be closed by this pull request
@mknorps mknorps force-pushed the 289-user-defined-mapping-add-option-for-multiple-mapping-files branch from a6b41a2 to 0a0546a Compare April 17, 2023 11:02
@mknorps mknorps changed the title WIP 289 user defined mapping add option for multiple mapping files User defined mapping: add option and handling for multiple mapping files Apr 17, 2023
@mknorps mknorps marked this pull request as ready for review April 17, 2023 11:11
@mknorps mknorps requested review from jherland and Nour-Mws and removed request for jherland April 17, 2023 11:11
Copy link
Member

@jherland jherland left a comment

Choose a reason for hiding this comment

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

Looks mostly good to me. My most important question is whether we can drop the Optional from Optional[Set[Path]]: I think an empty set will achieve the same as None?

fawltydeps/settings.py Outdated Show resolved Hide resolved
fawltydeps/packages.py Outdated Show resolved Hide resolved
fawltydeps/packages.py Outdated Show resolved Hide resolved
fawltydeps/packages.py Outdated Show resolved Hide resolved
fawltydeps/packages.py Outdated Show resolved Hide resolved
fawltydeps/packages.py Outdated Show resolved Hide resolved
fawltydeps/packages.py Outdated Show resolved Hide resolved
fawltydeps/packages.py Outdated Show resolved Hide resolved
If there is more than one custom mapping source (config or files) the resulting custom mapping is a combination of all of them. When a key (pakcage name) that is repeated (with respect to a normalised name) is encountered, all import names are gathered in a list
@mknorps mknorps force-pushed the 289-user-defined-mapping-add-option-for-multiple-mapping-files branch 3 times, most recently from 50ddda0 to cf2fa8f Compare April 18, 2023 09:16
Change 'custom_mapping_file' settings option to 'Set[Path]' and remove optional.

Use CustomMapping instead of Dict[str, List[str]]

Simplify custom mappings accumulation

drop unecessary comment

Co-authored-by: Johan Herland <johan.herland@tweag.io>
@mknorps mknorps force-pushed the 289-user-defined-mapping-add-option-for-multiple-mapping-files branch from cf2fa8f to 5762a36 Compare April 18, 2023 16:13
@mknorps mknorps merged commit fca8ecc into main Apr 18, 2023
23 checks passed
@mknorps mknorps deleted the 289-user-defined-mapping-add-option-for-multiple-mapping-files branch April 18, 2023 16:17
@mknorps mknorps linked an issue Apr 20, 2023 that may be closed by this pull request
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
2 participants