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

feat: Add store_full_path to converters (1/3) #8566

Merged
merged 9 commits into from
Nov 22, 2024

Conversation

Amnah199
Copy link
Contributor

@Amnah199 Amnah199 commented Nov 21, 2024

Related Issues

Proposed Changes:

Add a new parameter to the __init__ method to allow users to toggle whether the file_path in metadata stores the complete file path or only the base file name. A DeprecationWarning is generated about the upcoming change in file_path storage behavior in the next release.

How did you test it?

Added new test for checking the parameter function

Notes for the reviewer

This is 1/3 PRs that will add the same change to other converters.

Checklist

@github-actions github-actions bot added topic:tests type:documentation Improvements on the docs labels Nov 21, 2024
@coveralls
Copy link
Collaborator

coveralls commented Nov 21, 2024

Pull Request Test Coverage Report for Build 11972752772

Details

  • 0 of 0 changed or added relevant lines in 0 files are covered.
  • No unchanged relevant lines lost coverage.
  • Overall coverage increased (+0.03%) to 90.275%

Totals Coverage Status
Change from base Build 11972705853: 0.03%
Covered Lines: 7872
Relevant Lines: 8720

💛 - Coveralls

@Amnah199 Amnah199 changed the title feat: Add store_full_pathparameter to converters to toggle between store file path or file name feat: Add store_full_path to converters Nov 21, 2024
@Amnah199 Amnah199 changed the title feat: Add store_full_path to converters feat: Add store_full_path to converters (1/3) Nov 21, 2024
@Amnah199 Amnah199 marked this pull request as ready for review November 22, 2024 00:17
@Amnah199 Amnah199 requested review from a team as code owners November 22, 2024 00:17
@Amnah199 Amnah199 requested review from dfokina and julian-risch and removed request for a team November 22, 2024 00:17
Copy link
Member

@julian-risch julian-risch left a comment

Choose a reason for hiding this comment

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

The changes look quite good to me. I suggest that we move the store_full_path parameter from the run method to the init method instead. I don't see a need to change the store_full_path for different runs of the same component. If it turns out that's needed, we can add it later. For now, I would only add the parameter to the init method. Happy to discuss.

Once this is done and we merged the PR, could you please add a 2.9 milestone and add an issue about changing the default and removing the deprecation warning to the milestone please?

@Amnah199
Copy link
Contributor Author

I agree with having store_full_path in init. But do you think we should keep it in both? My reason to include it in run was that user could decide at the time of supplying sources param if he wanted to store only file names for some of the sources.

Copy link
Member

@julian-risch julian-risch left a comment

Choose a reason for hiding this comment

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

LGTM! 👍 We can add the parameter to the run methods to if we see requests for that. Right now, I don't see why a user would want to have the full path in meta for some files and only the file name for some other files in the same pipeline. Let's see.

@Amnah199 Amnah199 merged commit 21906d0 into main Nov 22, 2024
18 checks passed
@Amnah199 Amnah199 deleted the converters-add-store-full-path-param branch November 22, 2024 12:55
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
topic:tests type:documentation Improvements on the docs
Projects
None yet
Development

Successfully merging this pull request may close these issues.

3 participants