-
-
Notifications
You must be signed in to change notification settings - Fork 119
Fix README inaccuracies #480
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
Draft
cllns
wants to merge
26
commits into
main
Choose a base branch
from
fix-readme-inaccuracies
base: main
Could not load branches
Branch not found: {{ refName }}
Loading
Could not load tags
Nothing to show
Loading
Are you sure you want to change the base?
Some commits from the old base branch may be removed from the timeline,
and old review comments may become outdated.
Draft
Conversation
This file contains hidden or bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
Sign up for free
to join this conversation on GitHub.
Already have an account?
Sign in to comment
Add this suggestion to a batch that can be applied as a single commit.
This suggestion is invalid because no changes were made to the code.
Suggestions cannot be applied while the pull request is closed.
Suggestions cannot be applied while viewing a subset of changes.
Only one suggestion per line can be applied in a batch.
Add this suggestion to a batch that can be applied as a single commit.
Applying suggestions on deleted lines is not supported.
You must change the existing code in this line in order to create a valid suggestion.
Outdated suggestions cannot be applied.
This suggestion has been applied or marked resolved.
Suggestions cannot be applied from pending reviews.
Suggestions cannot be applied on multi-line comments.
Suggestions cannot be applied while the pull request is queued to merge.
Suggestion cannot be applied right now. Please check back later.
I used the README as a reference the other day and noticed it needed to be fixed. As I looked further, basically every section needed to be fixed or improved. It took a while to go through everything but I'm glad I did and we can have an accurate README. I went beyond just fixing the bare minimum and improved things as I went along. I found some bugs / potential improvements too. There's a good number of TODO's below, but I'd love feedback (and also check my work! I'm sure I made mistakes).
Summary
The biggest, nearly ubiquitous, fix was removing injected
configurationobjects, since we don't do that anymore.The other major change was to make all code examples self-contained. You should be able to copy and paste them directly into IRB and they should "Just Work". This means adding some extra irrelevant boilerplate (e.g.
ArticleRepoandArticle), but I think we can trust readers to ignore those.Minor changes:
# => [200, {}, ["body"]in many places but that's inaccurate. We get backHanami::Action::Responsenow. So users need to call#to_a(alias for#finish) to get the Rack-compatible array response. I started doing this throughout but I realized it could be broken up into several different lines for clarity, to only show the relevant info (via#status,#headers,#body, etc). This is more clear but also the headers used to be{}by default, and now they have Content-Type and Content-Length headers so it was very noisy (or had to be abbreviated as...which isn't valid Ruby here).pinstead ofputs. I was using both, depending on the situation, but it seemed distracting. The main issue is thatputsalways returnsnil, so#=> 123was describing the printed value instead of the return value. I'm open to changing this, I just wanted to be consistent.TODO
Structure
Rack
Formats
config.formats.addOther:
require "hanami/action"? Slightly confusing but we can add a note about it.Notes:
Bugs / Potential improvements:
I plan to open issues for all of these. I'll replace them with links to the Issues when I do that.
request.cookiesonly allow string key access (and it's a plain::Hash).request.sessionautomatically coerces keys to symbols, and can only be accessed via symbols only. We should rationalize these somehow, it's confusing they're so different.config.formats.add?expires 600, :public, max_age: 123adds two max-age values, e.g. Expires header ispublic, max-age=123, max-age=600. Maybe we can deprecate having both? I think I'd prefer max_age as a kwarg only and removing the first arg option.include Hanami::Action::Cookies(so I removed it from that example). However, setting cookies does need thatinclude, else you get a'Hash#fetch': key not found: "Set-Cookie" (KeyError)max_ageon a cookie automatically computes anExpiresas well. Using themax_agesetting from config does not set theExpires(you can see this in theSetCookiesexample)