Skip to content
This repository has been archived by the owner on Jul 3, 2023. It is now read-only.

remove unused loggers #103

Merged
merged 4 commits into from
Apr 11, 2022
Merged

remove unused loggers #103

merged 4 commits into from
Apr 11, 2022

Conversation

jameslamb
Copy link
Contributor

Looking through this project's code today, I found several places where module-specific loggers are instantiated but never used.

This PR proposes removing them

Changes

  • removes import of logging and instantiation of unused loggers in docs, examples, and the package itself

Testing

Ran the following to emulate CircleCI locally.

docker run \
   --rm \
    -v $(pwd):/usr/local/src \
    --workdir /usr/local/src \
   -it circleci/python:3.7 \
        /bin/bash

And then installed hamilton and ran its tests.

# install dependencies
sudo apt install graphviz
python -m venv venv || virtualenv venv
. venv/bin/activate
python --version
pip --version
pip install -r requirements-test.txt
pip install -r requirements.txt
pip install "dask[complete]"

# run tests
python -m pytest --cov=hamilton tests/
python -m pytest graph_adapter_tests/h_dask

Notes

  • removing unnecessary code from examples simplifies those examples and makes them more easier for new users to understand
  • removing unnecessary imports and module-level instantiations reduces the time it takes to run import hamilton
  • removing unused code reduces the time it takes to run static analyzers like mypy and flake8

Thanks for your time and consideration!

Checklist

  • PR has an informative and human-readable title (this will be pulled into the release notes)
  • Changes are limited to a single goal (no scope creep)
  • Code can be automatically merged (no conflicts)
  • Code passed the pre-commit check & code is left cleaner/nicer than when first encountered.
  • Passes all existing automated tests
  • Any change in functionality is tested
  • New functions are documented (with a description, list of inputs, and expected output)
  • Placeholder code is flagged / future TODOs are captured in comments
  • Project documentation has been updated if adding/changing functionality.
  • Reviewers requested with the Reviewers tool ➡️

Testing checklist

Python - local testing

  • python 3.6
  • python 3.7

Copy link
Collaborator

@elijahbenizzy elijahbenizzy left a comment

Choose a reason for hiding this comment

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

Nice! Can't believe we had so much cruft...

Copy link
Collaborator

@skrawcz skrawcz left a comment

Choose a reason for hiding this comment

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

I think we want to keep the logging setup lines. Since they control logging and are the entry point to running things. Otherwise yeah, if we're not using the loggers we should remove them -- can always add them back when we have something to log.

README.md Show resolved Hide resolved
examples/hello_world/my_script.py Show resolved Hide resolved
@jameslamb jameslamb requested a review from skrawcz March 30, 2022 14:36
@skrawcz
Copy link
Collaborator

skrawcz commented Apr 11, 2022

@jameslamb apologies for the slowness -- I miss github notifications often :/

@skrawcz skrawcz merged commit cd0a51c into stitchfix:main Apr 11, 2022
@jameslamb
Copy link
Contributor Author

ha no problem!

@jameslamb jameslamb deleted the unused-loggers branch April 11, 2022 13:31
Sign up for free to subscribe to this conversation on GitHub. Already have an account? Sign in.
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

None yet

3 participants