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

Simple cleanup #184

Open
wants to merge 1 commit into
base: master
Choose a base branch
from
Open

Conversation

ericfrederich
Copy link

@ericfrederich ericfrederich commented May 28, 2020

Hi, neat project. I'd like to contribute some features... but first, can we clean up this code?

There's a lot of changes to look through. There are no trojan-horses in this commit. I simply ran black on this repo. In fact you can reproduce it yourself.

git clone https://github.com/LazoCoder/Pokemon-Terminal.git
cd Pokemon-Terminal/
git remote add eric https://github.com/ericfrederich/Pokemon-Terminal.git
git fetch --all
# create new verification branch
git checkout -b verify origin/master 
# see tons of changes
git diff --stat eric/cleanup
# grab pre-commit configuration file from the cleanup branch
git show eric/cleanup:.pre-commit-config.yaml > .pre-commit-config.yaml
# run pre-commit
pre-commit run -a
# no difference now
git diff --stat eric/cleanup

These changes do nothing if you don't have pre-commit installed or have it installed but not enabled on your local repo, so it doesn't force this on anyone.

Copy link
Contributor

@jimmyorourke jimmyorourke left a comment

Choose a reason for hiding this comment

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

lgtm!
Personally I'm not the biggest fan of some of the ways that black formats things (and I don't like it messing with my quotes 😛 ) but I think the formatting changes here are definitely an improvement.

@ericfrederich
Copy link
Author

Personally I'm not the biggest fan of some of the ways that black formats things

I agree but I've come to accept it. When it becomes automatic via pre-commit then I don't even think about it much anymore.

@ericfrederich
Copy link
Author

Rebased on top of master

@sylveon
Copy link
Collaborator

sylveon commented Jun 5, 2020

This sounds pretty good to me, however I'm not sure if the changes pokemonterminal/Data/pokemon.txt won't break anything

@ericfrederich
Copy link
Author

@sylveon let me ease your concerns.

  • I've been using my copy of this code without issue. Installed via pipx install -e . while in my Git repo.
  • Doing a side-by-side diff on the .txt file shows only trailing whitespace was stripped. This was already being done by the python code pkmn_data = line.strip().split() seen here

@jimmyorourke
Copy link
Contributor

I say if it works 🚢 it!

But if somehow the trailing whitespace removal from pokemon.txt does cause problems then we really shouldn't be using a txt file, and should be using something standard like csv or json instead

Copy link
Collaborator

@sylveon sylveon left a comment

Choose a reason for hiding this comment

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

Sounds good :shipit:

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.

3 participants