-
Notifications
You must be signed in to change notification settings - Fork 138
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
Conversation
There was a problem hiding this 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.
-
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!
I wanted to figure out why the build problem was caused when I commited a change that added 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 This why I added an exclusion list for fonts that cause an error. |
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. |
Thanks for filing anyway, I made the readme into well formed markdown in time for 1.0. |
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..