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

Simplify Python Code #698

Merged
merged 15 commits into from
Apr 2, 2022
Merged

Simplify Python Code #698

merged 15 commits into from
Apr 2, 2022

Conversation

MartinThoma
Copy link
Collaborator

@MartinThoma MartinThoma commented Apr 2, 2022

Another bigger one, fixing many tiny issues (and a few bugs)

print_with_tab / print_with_whitespace is trivial with Python
string formatting and was mostly used in only 2 lines.
@MartinThoma MartinThoma force-pushed the py-simplify branch 3 times, most recently from dddb254 to b3aa8ff Compare April 2, 2022 12:12
Copy link
Collaborator

@mojoaxel mojoaxel left a comment

Choose a reason for hiding this comment

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

It looks to me like this mainly is about coding-conventions. I personally would feel pissed if someone would re-format my code without any written coding-convention. I would prefer if we would leave code-style to the person who took the time to implement a game!?

@MartinThoma
Copy link
Collaborator Author

MartinThoma commented Apr 2, 2022

I would prefer if we would leave code-style to the person who took the time to implement a game!?

Could you please be more specific / give an example what you mean? I've changed a lot in this PR:

  • 27 Civil War: A quite massive change
  • 35 Even wins: Implementing a change the original author has pointed to
  • 70 Poetry: Remove usage of globals; a comment of the author pointed to the fact that they weren't too happy with the solution before
  • In many files: Convert print("") and input("") to print() and input()
  • In many files: Type annotations (which was acceptable (or at least accepted) before
  • Remove many print_at_column / print_with_tab in favor of the default Python string features
  • Some play function were renamed to main to be consistent over all games
  • Some snakeCase -> camel_case to be consistent with PEP8 (PEP8 is the de-facto coding convention)

I know that some people have strong feelings about coding style changes, but I don't really understand that. It's a community project. If people wan't to keep their style, they can do that in their repository. I think a maintainer should strive for a consistent coding style that makes it easy for others to understand + change code in a project, especially when there are likely many beginners looking at it.

If we make a "who wrote it, owns it" policy, we essentially have one owner per file. That would make changes like #699 impossible.

@mojoaxel
Copy link
Collaborator

mojoaxel commented Apr 2, 2022

Thanks for all your useful contributions! Why not do them in a seperate merge-requests? It's always hard to discuss these massive changes!

If we make a "who wrote it, owns it" policy, we essentially have one owner per file.

That is not my intention! Any useful change should be done!
But, as a long-time maintainer I made the experience that it DOES make a difference who wrote a file, showes up in a git blame and who is on top of the contributor list.

That's why I personally think open repositories with multiple contributors should stick to the following rules:

  • do not just rename files! Use git mv to rename them!
  • do not change lines that don't NEED changing in you current pull-request.
  • don't merge your own merge-request without a review.
  • do not enforce your coding style. Let the maintainers decide IF they want a coding style and then let's enforce it using bots, build-hooks or the original contributors!

But to be honest, I don't care that much about python. I guess it's for other people to decide!

@MartinThoma
Copy link
Collaborator Author

it DOES make a difference who wrote a file, showes up in a git blame

Ah, I see! I can understand that. I think a core point to recognize is who originally did the work of porting the implementation of a game to the specific language. I see that most (all?) people put themselves somewhere in the file. I thought that would be enough for recognizing / honoring the work that initial contributor put in it.

I see several ways a contribution is honored / recognized

  • Number of commits (automatically shown by Github somewhere; I don't remember where)
  • The file itself, e.g. "originally ported to language XY by Z" (such comments should always be left intact! I hope I didn't accidentially delete one of them?)
  • the commit history (that is one reason why I was hesitant to re-write it and made sure that I didn't delete any source files from the history)

do not just rename files! Use git mv to rename them!

Sadly, that doesn't always help. The only way I see to ensure that renaming doesn't drop the git history is to make two commits: (1) renaming (2) adjusting the contents.

Especially when I squash commits / git commit --amend, then it sometimes happens that it's not recognized that it was a rename + change.

do not change lines that don't NEED changing in you current pull-request.

do not enforce your coding style. Let the maintainers decide IF they want a coding style and then let's enforce it using bots, build-hooks or the original contributors!

I feel like those two are really hard to judge in many cases. As an example: You could argue that the changes I made in https://github.com/coding-horror/basic-computer-games/pull/696/files were not necessary. You could also say that those are just coding style changes - in the end, the program does the same thing (same user input/random state leads to same output of the program). However, I would argue that the PR improved the code quite a bit. It would also not be something that can be enforced using bots, as such structural changes are way too complex for bots.

@MartinThoma
Copy link
Collaborator Author

Btw: I think Python has less extreme linters for enforcing small rules than JS has. But I'm working on that part with my plugin flake8-simplify 😇 😄

@coding-horror
Copy link
Owner

I think as long as the initial contributor is honored, perhaps in the comments of the program, that's best? It's always visible in Git history at least?

@coding-horror coding-horror merged commit 60029f7 into main Apr 2, 2022
@coding-horror coding-horror deleted the py-simplify branch April 2, 2022 19:13
@coding-horror
Copy link
Owner

Also @mojoaxel and @MartinThoma could you email me your postal addresses? I wanted to mail you a little something in the postal mail to thank you for the time you are contributing to this project! 💌

rjrw pushed a commit to rjrw/basic-computer-games that referenced this pull request Dec 19, 2022
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