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

DateTimeColumn: Handle Datetime day suffixes #458

Merged
merged 15 commits into from
Apr 13, 2022
Merged

DateTimeColumn: Handle Datetime day suffixes #458

merged 15 commits into from
Apr 13, 2022

Conversation

micdavis
Copy link
Contributor

Fix: Updated datetime_column_profile.py to remove day suffixes from the date when we check its format. Updated the test_datetime_column_profile.py test to test for this change. Addresses this issue #441

Copy link
Contributor Author

@micdavis micdavis left a comment

Choose a reason for hiding this comment

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

One potential issue is that if a date is formatted as such:
15thMarth13 then it will be picked up as 2013-03-15.

Copy link
Contributor

@taylorfturner taylorfturner left a comment

Choose a reason for hiding this comment

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

one syntax comment

dataprofiler/profilers/datetime_column_profile.py Outdated Show resolved Hide resolved
@JGSweets JGSweets enabled auto-merge (squash) April 12, 2022 19:33
Copy link
Collaborator

@JGSweets JGSweets left a comment

Choose a reason for hiding this comment

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

LGTM, small fixes

@taylorfturner
Copy link
Contributor

@micdavis , run isort . in the root of the repo just to make sure those imports are sorted correctly

JGSweets
JGSweets previously approved these changes Apr 13, 2022
@@ -268,6 +268,8 @@ def test_profile(self):
expected = defaultdict(float, {'datetime': 2.0})
self.assertEqual(expected, profiler.profile['times'])

Copy link
Contributor

Choose a reason for hiding this comment

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

nit pick but you can probably get rid of this extra space between the two self.assert in the same test function

Copy link
Contributor

@taylorfturner taylorfturner left a comment

Choose a reason for hiding this comment

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

  • one specific comment
  • one general for isort .
    other than those, I'll apporove

@JGSweets JGSweets merged commit de3e214 into capitalone:main Apr 13, 2022
stevensecreti pushed a commit to stevensecreti/DataProfiler that referenced this pull request Jun 15, 2022
* Fix: updated test_(int/float)_column_profile.py test_diff to cover json.dumps() functionality. Updated numerical_column_stats.py _perform_t_test to cast conservative and welch values to floats before assignment.

* Fix: addressed PR change requests.

* Fix: Reformatting

* Fix: Added check to test to cover newly required functionality.

* Bumped version

* Fixed Datetime profiler day suffix issue
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.

None yet

3 participants