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

Updated setup.cfg with check-manifest #705

Merged
merged 12 commits into from
Nov 3, 2022

Conversation

Sanketh7
Copy link
Contributor

@Sanketh7 Sanketh7 commented Nov 2, 2022

Adding check-manifest as hook for pre-commit

@taylorfturner taylorfturner added the High Priority Dramatic improvement, inaccurate calculation(s) or bug / feature making the library unusable label Nov 2, 2022
@taylorfturner
Copy link
Contributor

@Sanketh7 needs an update branch on this FYI

setup.cfg Show resolved Hide resolved
@taylorfturner taylorfturner self-requested a review November 2, 2022 19:26
MANIFEST.in Outdated
Comment on lines 15 to 17
recursive-include examples *.ipynb
recursive-include examples *.md
recursive-include examples *.png
Copy link
Contributor

Choose a reason for hiding this comment

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

concerned these are included in the installation... they shouldn't be

Copy link
Contributor

Choose a reason for hiding this comment

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

that is ipynb, md, and png

Copy link
Contributor

Choose a reason for hiding this comment

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

if I take them out I receive

missing from sdist:
  examples/DL-Flowchart.png
  examples/add_new_model_to_data_labeler.ipynb
  examples/column_name_labeler.ipynb
  examples/data_profiler_demo.ipynb
  examples/data_readers.ipynb
  examples/graph_data_demo.ipynb
  examples/great_expecations/README.md
  examples/great_expecations/ge_expect_column_value_confidence_for_data_label.ipynb
  examples/great_expecations/ge_expect_column_values_vs_profile_min.ipynb
  examples/great_expecations/ge_expect_profile_numeric_columns_diff_in_range.ipynb
  examples/great_expecations/ge_expect_profile_numeric_columns_percent_diff_in_range.ipynb
  examples/intro_data_profiler.ipynb
  examples/labeler.ipynb
  examples/merge_profile_list.ipynb
  examples/popmon_dp_loader_example.ipynb
  examples/regex_labeler_from_scratch/DataLabeler_from_scratch.ipynb
  examples/structured_profilers.ipynb
  examples/unstructured_profilers.ipynb

and check-manifest recommends adding those 3 lines (15 - 17)

Comment on lines +48 to +57
# Check-manifest: ensures required non-Python files are included in MANIFEST.in
# https://github.com/mgedmin/check-manifest/blob/master/.pre-commit-hooks.yaml
- repo: https://github.com/mgedmin/check-manifest
rev: "0.48"
hooks:
- id: check-manifest
additional_dependencies: ['h5py', 'wheel', 'future', 'numpy', 'pandas',
'python-dateutil', 'pytz', 'six', 'pyarrow', 'chardet', 'fastavro',
'python-snappy', 'charset-normalizer', 'psutil', 'scipy', 'requests',
'networkx','typing-extensions']
Copy link
Contributor

Choose a reason for hiding this comment

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

run check-manifest on pre-commit run

setup.cfg Outdated
Comment on lines 17 to 21
DataProfiler.egg-info/**
dataprofiler/reports/.mypy_cache/**
dataprofiler/tests/data/json/honeypot
dataprofiler/labelers/embeddings/glove-reduced-64D.txt
resources/__pycache__/__init__.cpython**
Copy link
Contributor

Choose a reason for hiding this comment

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

had to add these for check-manifest to pass

Copy link
Contributor

Choose a reason for hiding this comment

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

that is lines 17 - 21

@taylorfturner taylorfturner added New Feature A feature addition not currently in the library dependencies Pull requests that update a dependency file labels Nov 2, 2022
@taylorfturner taylorfturner self-requested a review November 2, 2022 19:31
@@ -1,3 +1,4 @@
check-manifest>=0.48
Copy link
Contributor

Choose a reason for hiding this comment

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

adding new dependency of check-manifest for pre-commit

Comment on lines +54 to +57
additional_dependencies: ['h5py', 'wheel', 'future', 'numpy', 'pandas',
'python-dateutil', 'pytz', 'six', 'pyarrow', 'chardet', 'fastavro',
'python-snappy', 'charset-normalizer', 'psutil', 'scipy', 'requests',
'networkx','typing-extensions']
Copy link
Contributor

Choose a reason for hiding this comment

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

have to add these for check-manifest to run in the pre-commit environment

Copy link
Contributor

Choose a reason for hiding this comment

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

read this but was reading a post from the creator of pre-commit that using -r requirements.txt in the add_dependencies is not good practice

@taylorfturner taylorfturner self-requested a review November 2, 2022 20:42
@JGSweets JGSweets enabled auto-merge (squash) November 2, 2022 21:00
@taylorfturner taylorfturner dismissed their stale review November 2, 2022 21:53

dismissing my own

@JGSweets JGSweets merged commit 948816f into capitalone:main Nov 3, 2022
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
dependencies Pull requests that update a dependency file High Priority Dramatic improvement, inaccurate calculation(s) or bug / feature making the library unusable New Feature A feature addition not currently in the library
Projects
None yet
Development

Successfully merging this pull request may close these issues.

4 participants