Fix typo preventing access to response object on error #62
Conversation
|
The same tests are failing on master branch |
CHANGELOG.md
Outdated
| @@ -1,6 +1,6 @@ | |||
| ### 0.3.5 (Next) | |||
|
|
|||
| * Your contribution here. | |||
There was a problem hiding this comment.
Could you bring this back to the CHANGELOG? This way new contributors will be able to easily see that a new entry should be added here.
| @@ -1,7 +1,7 @@ | |||
| module Graphlient | |||
| module Errors | |||
| class GraphQLError < Error | |||
| attr_reader :responsee | |||
| attr_reader :response | |||
There was a problem hiding this comment.
While this is a just typo, this is technically a breaking change and could break existing users and it should ho through the deprecation cycle.
There was a problem hiding this comment.
good point. is there a deprecation guide? i know i should add back the old attr_reader but is there any particular way i should mark this for deprecation?
There was a problem hiding this comment.
I believe we follow semver in terms of versioning. Then the old method could output a warning when called:
class GraphQLError < Error
attr_reader :response
...
def responsee
warn "The `#responsee' method is deprecated since it has a typo. Please use the `#response' method instead."
response
end
end@ashkan18 Is this something you would like to do? Do you have any preferences?
There was a problem hiding this comment.
added a deprecation notice. i returned @responsee though (always nil) since that was the old behavior.
There was a problem hiding this comment.
Oh I did not notice that it always returns nil. That means it is less likely that people actually are using it and we do not have to worry about compatibility. I am sorry for the back and forth, but would you just remove it? Renaming should just be fine.
| warn "The `#responsee' method is deprecated since it has a typo. Please use the `#response' method instead." | ||
|
|
||
| @responsee | ||
| end |
There was a problem hiding this comment.
I think you have meant to add this to lib/graphlient/errors/graphql_error.rb.
There was a problem hiding this comment.
right, thanks. my multitasking isn't going too well here ;)
|
thanks @jmondo for 👀 , I fixed the issue on |
|
@ashkan18 looks like we're green! |
|
thanks @jmondo for your contribution 💜 |
|
just cut |
|
Whoa nice thanks for cutting the release so quickly :) I can remove my
monkey patch! Haha
On Tue, Jul 23, 2019 at 05:02 Ashkan Nasseri ***@***.***> wrote:
just cut 0.3.6 with this change 🙌
—
You are receiving this because you were mentioned.
Reply to this email directly, view it on GitHub
<#62?email_source=notifications&email_token=AAETKJ3MDM5XZ4AC5GNLPSDQA3XNRA5CNFSM4IF4Y2LKYY3PNVWWK3TUL52HS4DFVREXG43VMVBW63LNMVXHJKTDN5WW2ZLOORPWSZGOD2S4KPI#issuecomment-514180413>,
or mute the thread
<https://github.com/notifications/unsubscribe-auth/AAETKJ4OCFMUQDFN4OTXPITQA3XNRANCNFSM4IF4Y2LA>
.
--
-- John
|
No description provided.