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

Refactor SAPBW source #1066

Open
3 tasks
angelika233 opened this issue Sep 30, 2024 · 2 comments
Open
3 tasks

Refactor SAPBW source #1066

angelika233 opened this issue Sep 30, 2024 · 2 comments
Labels
2.0 viadot 2.0 enhancement New feature or request

Comments

@angelika233
Copy link
Contributor

angelika233 commented Sep 30, 2024

  • Check if _apply_user_mapping() is needed and if possible replace it with df.rename(columns=mapping_dict).

  • api_connection() should be refactored into to_records() or just included as part of to_df()

  • add unit tests

#1053 (comment)

@angelika233
Copy link
Contributor Author

remove mapping_dict from to_df()

@trymzet
Copy link
Contributor

trymzet commented Sep 30, 2024

Seems like this _apply_mapping is actually doing 2 things: first it filters a dataframe, and then renames the columns. Eg. if we have a filter + mapping dict like this: {"my_col": "my_new_col_name"} and a df with 2 columns, "my_col" and "my_other_col", after applying this function, we'd get a df with 1 column, "my_new_col_name".

So I guess this function is confusing for 3 reasons:

  • it does 2 things
  • the naming is misleading, as it only mentions one of the two things it's doing
  • the docstring doesn't explain what this func is doing, ony uses a generic term "applies mapping"

So it should probably be split into 2 functions or just native dataframe functions used (filtering a df and renaming columns are done with a single line of code in core pandas, so it's likely wrappers are not needed).

@trymzet trymzet added 2.0 viadot 2.0 enhancement New feature or request labels Sep 30, 2024
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
2.0 viadot 2.0 enhancement New feature or request
Projects
None yet
Development

No branches or pull requests

2 participants