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

Remade README, Other changes #74

Closed
wants to merge 10 commits into from
Closed

Remade README, Other changes #74

wants to merge 10 commits into from

Conversation

Sxnic
Copy link

@Sxnic Sxnic commented Jan 15, 2020

I have completely remade the readme to make it a markdown file, made the CHANGELOG a separate file, added long_description to setup.py, so that people can view the description of pyfiglet directly from pypi, and added a formatter for optparse to make the help screen look nicer.

(I also added .tlf files to the setup.py, becuase that extension wasn't in package_data)

Also, this is my first time doing a pull request, so I hope I did everything right..

Copy link
Owner

@pwaller pwaller left a comment

Choose a reason for hiding this comment

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

Hi @Sxnic, thanks for taking the time to contribute! I appreciate it, and have taken some of my limited time to respond. Sorry it's not an instant-accept, please don't take this as a negative :)

I would have preferred many smaller PRs rather than one big PR, but let's go with what you have if you prefer. However, "Big PR" means "big comments", and perhaps harder to complete the review.

I assume you're familiar with how to edit your git history and submit things as separate PRs. If not, I'm happy to teach you how -- or do it for you. Just let me know.

The readme and changelog improvements look solid.

Problems in order of priority:

  • The tests appear to be failing. Are you able to investigate the details and hopefully fix this? I haven't looked into it.

    Screenshot from 2020-01-17 20-36-45

  • I'm uneasy about putting the source code of another project with a different license or introducing any other dependencies. If we were going to do this I would need some strong reasons to do so. I don't have the time to audit the optparse_pretty thoroughly myself at the moment and don't see any strong trust signals for the origin of the code. I suggest separating that out into an issue or PR for further discussion, but there is a lot of persuading to do to get me to accept it. Please make clear what improvement the change brings. (Perhaps with screenshots, etc?)

  • The title of this PR doesn't mention one of the major changes you made! You should say "Add color to the help text" or similar. Likewise "Change README format from .rst to .md". This makes these changes easier to identify for other people (or yourself in the distant future).

  • ac82e65 has the commit title "General changes", but it appears to introduce the newly colored help text. A commit introducing such a change should clearly say so (and preferably not also introduce minor syntactic changes at the same time). But please consider that I might not want to accept this change at this time, as with the above bullet.

Happy hacking!

@Sxnic
Copy link
Author

Sxnic commented Jan 21, 2020

I wanted to figure out why the build problem was caused when I commited a change that added .tlf files to the package data.

It turns out some files are unreadable by (for example) visual studio code. While the original TOIlet source doesn't show how to properly decode those files. And this might be the reason why .tlf files were excluded from the list before I commited my first changes.

This why I added an exclusion list for fonts that cause an error.
When I commited the new change, it seems like the build passes again. (strange)

@jkob jkob mentioned this pull request Apr 17, 2020
@pwaller
Copy link
Owner

pwaller commented Nov 1, 2020

Please can you break this into smaller PRs? I really like the readme change, for example. The smaller the scope of the PR the better since it makes it easy to review and accept, and then it's less likely to be rejected or wait for a long time. And if it is rejected, it's a smaller amount of stuff being rejected.

As it stands, I'm sorry but I can't accept this PR without major changes. I made a few recommendations in an earlier comment you haven't touched on, and adding code under a different license is a showstopper for the time being.

@pwaller pwaller closed this Nov 1, 2020
@pwaller
Copy link
Owner

pwaller commented Sep 1, 2023

Thanks for filing anyway, I made the readme into well formed markdown in time for 1.0.

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.

None yet

2 participants