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

[#38] Adding Black Formatting #39

Merged
merged 4 commits into from
Oct 9, 2020
Merged

Conversation

apoclyps
Copy link
Contributor

@apoclyps apoclyps commented Oct 6, 2020

What's Changed

Apply's black formatting to the codebase

Technical Description

  • Removes unused imports
  • Fixes typo in Exception
  • Adds whitespace to break up sections
  • Applies Black Formatting
  • Creates requirements for the project

Closes #38

Copy link
Owner

@mackorone mackorone left a comment

Choose a reason for hiding this comment

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

Looks good! It goes without saying, but thank you for breaking up your commits. Much easier to review!

@@ -538,7 +584,7 @@ def push_updates(now):
print("Removing origin")
remote_rm = run(["git", "remote", "rm", "origin"])
if remote_rm.returncode != 0:
raise Excetion("Failed to remove origin")
raise Exception("Failed to remove origin")
Copy link
Owner

Choose a reason for hiding this comment

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

Good catch!

Comment on lines +11 to +12
import requests

Copy link
Owner

Choose a reason for hiding this comment

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

I installed requests so long ago, I forgot it wasn't a builtin! Thanks for rearranging these.

Copy link
Owner

@mackorone mackorone left a comment

Choose a reason for hiding this comment

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

Looks good to me!

Copy link
Owner

@mackorone mackorone left a comment

Choose a reason for hiding this comment

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

This is going to sound silly, but I'm new to requirements.in. Can you give me a quick crash course or a pointer to some documentation? How is it different from requirements.txt and when should I use one versus the other?

Also, I searched for requirements_dev.txt and requirements-dev.txt was returned: https://pypi.org/project/requirements-dev.txt/. Is the latter more conventional?

toml==0.10.1 # via black
typed-ast==1.4.1 # via black
typing-extensions==3.7.4.3 # via black
urllib3==1.25.10 # via requests
Copy link
Owner

Choose a reason for hiding this comment

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

Silly question: these comments were autogenerated, right?

Copy link
Contributor Author

Choose a reason for hiding this comment

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

All comment's within the requirements.txt files are autogenerated when running the corresponding pip-compile requirements.in command. As more transitive dependencies are added and more direct dependencies depending on a specific version, the direct dependency will also be added to the comment.

Comment on lines 1 to 10
# Shared/Runtime dependencies

-r requirements.in

# Development-only dependencies


# Lint-only dependencies

black==20.8b1
Copy link
Owner

Choose a reason for hiding this comment

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

Is this the standard requirements_dev.in template?

Copy link
Contributor Author

@apoclyps apoclyps Oct 8, 2020

Choose a reason for hiding this comment

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

Not quiet;

pip-compile --output-file=requirements-dev.txt requirements-dev.in

would produce

#
# This file is autogenerated by pip-compile
# To update, run:
#
#    pip-compile --output-file=requirements-dev.txt requirements-dev.in
#
appdirs==1.4.4            # via black
black==20.8b1             # via -r requirements-dev.in
certifi==2020.6.20        # via requests
chardet==3.0.4            # via requests
click==7.1.2              # via black
idna==2.10                # via requests
mypy-extensions==0.4.3    # via black
pathspec==0.8.0           # via black
regex==2020.9.27          # via black
requests==2.24.0          # via -r requirements.in
toml==0.10.1              # via black
typed-ast==1.4.1          # via black
typing-extensions==3.7.4.3  # via black
urllib3==1.25.10          # via requests

However, adding -r requirements.in to the start of the dev dependencies avoids listing the entire contents of requirements.txt.

The comments that appear after this line are not necessary (personal preference) but it helps provide a logical grouping to identify what dependencies belong too. Useful if/when more are added.

# Development-only dependencies


# Lint-only dependencies

black==20.8b1

I'm happy to make changes around any of this if it feels inappropriate for the project.

@mackorone
Copy link
Owner

I did a bit more digging (https://github.com/jazzband/pip-tools). Is the difference between requirements.in and requirements.txt that the former is the list of direct dependencies, and the latter a list of transitive dependencies?

@apoclyps
Copy link
Contributor Author

apoclyps commented Oct 8, 2020

yes, that's correct. requirements.in for direct dependencies and requirements.txt for transitive dependencies. I'm happy to change this approach to a vanilla requirements.txt.

I appreciate the project doesn't have very many dependencies and this approach may be more than what is necessary. I would like to know your thoughts :)

@mackorone
Copy link
Owner

@apoclyps thanks for the thoughtful responses. I think it's fine to keep this as is. There's no time like the present to adopt best practices! I'll go ahead and merge this.

@mackorone mackorone merged commit 13edc51 into mackorone:master Oct 9, 2020
@apoclyps apoclyps deleted the formatting branch October 9, 2020 10:10
github-actions bot pushed a commit to vitokorn/spotify-playlist-archive that referenced this pull request Mar 24, 2023
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.

Format with Black
2 participants