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.

Sign up for GitHub

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

Jump to bottom

Applying isort and resolving circular imports #453

Merged
merged 5 commits into from
Apr 6, 2022
Merged

Applying isort and resolving circular imports #453

merged 5 commits into from
Apr 6, 2022

Conversation

taylorfturner
Copy link
Contributor

@taylorfturner taylorfturner commented Apr 6, 2022 •
edited

  • Applying isort to the respository
  • Resolving circular imports

@taylorfturner taylorfturner added the Bug Something isn't working label Apr 6, 2022
@taylorfturner taylorfturner self-assigned this Apr 6, 2022
@JGSweets JGSweets enabled auto-merge (squash) April 6, 2022 03:06
@taylorfturner taylorfturner changed the title isort Applying isort and resolving circular imports Apr 6, 2022
JGSweets
JGSweets reviewed Apr 6, 2022
View reviewed changes
.isort.cfg Outdated Show resolved Hide resolved
JGSweets
JGSweets reviewed Apr 6, 2022
View reviewed changes
dataprofiler/tests/labelers/test_unstructured_data_labeler.py Outdated
Comment on lines 12 to 13
from dataprofiler.data_readers.csv_data import CSVData
from dataprofiler.data_readers.csv_data import JSONData
from dataprofiler.data_readers.csv_data import ParquetData
from dataprofiler.data_readers.csv_data import AVROData

from dataprofiler.labelers import data_processing, DataLabeler, \
UnstructuredDataLabeler
from dataprofiler.data_readers.csv_data import (AVROData, CSVData, JSONData,
ParquetData)
Copy link
Collaborator

Choose a reason for hiding this comment

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

this is a weird import. the fact we are importing through csv_data as opposed to:

from dataprofiler.data_readers.avro_data import AVROData
from dataprofiler.data_readers.base_data import BaseData
from dataprofiler.data_readers.json_data import JSONData
from dataprofiler.data_readers.parquet_data import ParquetData

Copy link
Collaborator

Choose a reason for hiding this comment

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

or at least I could see making these avail through the __init__ in data_readers

Copy link
Contributor Author

Choose a reason for hiding this comment

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

very weird ... will check on this

JGSweets
JGSweets reviewed Apr 6, 2022
View reviewed changes
dataprofiler/tests/profilers/profiler_options/test_base_option.py
@@ -1,8 +1,8 @@
import unittest

from dataprofiler.profilers.profiler_options import BaseOption
from dataprofiler.tests.profilers.profiler_options.abstract_test_options \
import AbstractTestOptions
from dataprofiler.tests.profilers.profiler_options.abstract_test_options import \
Copy link
Collaborator

Choose a reason for hiding this comment

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

so this now breaches the 80 char limit, so whats going to happen if both flake8 and isort disagree?

JGSweets
JGSweets reviewed Apr 6, 2022
View reviewed changes
dataprofiler/tests/profilers/profiler_options/test_categorical_options.py
@@ -1,6 +1,6 @@
from dataprofiler.profilers.profiler_options import CategoricalOptions
from dataprofiler.tests.profilers.profiler_options.test_base_inspector_options \
import TestBaseInspectorOptions
from dataprofiler.tests.profilers.profiler_options.test_base_inspector_options import \
Copy link
Collaborator

Choose a reason for hiding this comment

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

since these are in tests, should we just be calling the tests relative as oppose to from the base of the dataprofiler.

e.g. tests are relative, but repo itself is not? would fix these 80 char issues

JGSweets
JGSweets reviewed Apr 6, 2022
View reviewed changes
dataprofiler/tests/profilers/profiler_options/test_datalabeler_options.py
from dataprofiler.labelers.base_data_labeler import BaseDataLabeler
from dataprofiler.profilers.profiler_options import DataLabelerOptions
from dataprofiler.tests.profilers.profiler_options.test_base_inspector_options import \
Copy link
Collaborator

Choose a reason for hiding this comment

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

as above

JGSweets
JGSweets reviewed Apr 6, 2022
View reviewed changes
dataprofiler/tests/profilers/profiler_options/test_datetime_options.py
@@ -1,6 +1,6 @@
from dataprofiler.profilers.profiler_options import DateTimeOptions
from dataprofiler.tests.profilers.profiler_options.test_base_inspector_options \
import TestBaseInspectorOptions
from dataprofiler.tests.profilers.profiler_options.test_base_inspector_options import \
Copy link
Collaborator

Choose a reason for hiding this comment

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

as above

@JGSweets JGSweets merged commit 1a7bd77 into capitalone:main Apr 6, 2022
@taylorfturner taylorfturner deleted the bug_fix/imports branch April 6, 2022 14:51
stevensecreti pushed a commit to stevensecreti/DataProfiler that referenced this pull request Jun 15, 2022
@taylorfturner
isort (capitalone#453)
db101c8 
* isort

* filepath function issue
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Reviewers

@JGSweets JGSweets JGSweets approved these changes

@ksneab7 ksneab7 ksneab7 approved these changes

@grant-eden grant-eden Awaiting requested review from grant-eden

@ChrisWallace2020 ChrisWallace2020 Awaiting requested review from ChrisWallace2020

Assignees

@taylorfturner taylorfturner

Labels
Bug Something isn't working
Projects
None yet
Milestone
No milestone
Development

Successfully merging this pull request may close these issues.

None yet
3 participants
@taylorfturner @JGSweets @ksneab7