-
Notifications
You must be signed in to change notification settings - Fork 87
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
Minor code improvements #110
Minor code improvements #110
Conversation
Removed some unnecessary comments and rewrote repeated reassignments to method chaining.
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.
This is really great, thank you very much!
I'll say that the changes to ErrorPages
don't constitute a breaking change because from looking at them I don't think they should break any user-facing code (and custom engines aren't supported yet), is that correct?
This seems to have some conflicts, are you able to resolve those? |
Alright, merge conflicts resolved. Actually, I think this might be a breaking change because of the parameter type change from |
I genuinely have no clue why that comment was in italics, sometimes the GitHub mobile app is magical. I don't see any changes to the parameters that examples take, I think they've always taken owned values haven't they? |
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.
If this isn't a breaking change, it's good to merge.
Yeah @Asha20 I'm pretty sure you've just needed to change some things in |
No description provided.