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: simplify columns_cleanups using regexs #172

Merged
merged 5 commits into from
Apr 27, 2020
Merged

Refactor: simplify columns_cleanups using regexs #172

merged 5 commits into from
Apr 27, 2020

Conversation

jflairie
Copy link
Contributor

Description

This PR is intended to simplify the cleanup of column names by generating a regular expression based on 2 variables:

  • a list of all the characters to replace. By default, only letters will be kept
  • the character to replace with. By default _

Note that this PR paves the way for introducing custom cleanup parameters. So next step is to parse new configurations

How has this change been tested?

on ipython with a dummy df

Checklist

  • The PR is named correctly according CONTRIBUTE.md standards.
  • My code follows the style guidelines (black formatting, typing etc.)
  • I have performed a self-review of my own code
  • I have commented my code, particularly in hard-to-understand areas
  • [] I have made corresponding changes to the documentation
  • I have tested that my code works
  • In case of breaking changes, I have provided enough notice or refactored code using the feature I have just changed

Copy link
Owner

@bastienboutonnet bastienboutonnet left a comment

Choose a reason for hiding this comment

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

Thanks a lot for picking up this issue and cleaning up the part that's supposed to clean the sheet up, but was itself written dirty. Oh the irony!

I really like what you're doing here! I have a few pointers to make things a bit more robust and clearer but you're totally on the right track.

I would suggest you config mypy and flake8 as your linters so that they will complain when they should. For example the typing issue I brought up would have been flagged. Also, mypy as a linter is super nice!

core/cleaner.py Show resolved Hide resolved
core/cleaner.py Outdated Show resolved Hide resolved
core/cleaner.py Outdated Show resolved Hide resolved
core/cleaner.py Outdated Show resolved Hide resolved
core/cleaner.py Outdated Show resolved Hide resolved
@bastienboutonnet bastienboutonnet changed the base branch from dev/nicolas_jaar to dev/objekt April 26, 2020 20:11
@bastienboutonnet bastienboutonnet changed the base branch from dev/objekt to dev/nicolas_jaar April 26, 2020 20:13
@bastienboutonnet bastienboutonnet changed the base branch from dev/nicolas_jaar to dev/objekt April 26, 2020 20:13
@sonarqubecloud
Copy link

Kudos, SonarCloud Quality Gate passed!

Bug A 0 Bugs
Vulnerability A 0 Vulnerabilities (and Security Hotspot 0 Security Hotspots to review)
Code Smell A 0 Code Smells

No Coverage information No Coverage information
0.0% 0.0% Duplication

Copy link
Owner

@bastienboutonnet bastienboutonnet left a comment

Choose a reason for hiding this comment

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

Awesome! Looks great @jflairie 🎉 I'm gonna go ahead and approve it. I'll merge it into dev/objekt. Feel free to pick up on reading the config if you want to give user control (Issue #165)

@bastienboutonnet bastienboutonnet added this to the Objekt milestone Apr 27, 2020
@bastienboutonnet
Copy link
Owner

Closes #35

@bastienboutonnet bastienboutonnet changed the title Refactor: simplify columns_cleanups using regular expression generator Refactor: simplify columns_cleanups using regexs Apr 27, 2020
@bastienboutonnet bastienboutonnet merged commit 64c79a9 into bastienboutonnet:dev/objekt Apr 27, 2020
@jflairie jflairie deleted the refactor_cleanup_steps branch April 27, 2020 11:45
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.

2 participants