Skip to content

Conversation

@styd
Copy link
Contributor

@styd styd commented Apr 30, 2017

When a new class inherits from the AccessToken class, its refresh! method should return an instance of its own class, not of AccessToken's.

@styd styd closed this May 3, 2017
@styd styd force-pushed the extending-access-token-class branch from 46e67f5 to 96daaf5 Compare May 3, 2017 15:14
@styd styd reopened this May 3, 2017
@coveralls
Copy link

coveralls commented May 3, 2017

Coverage Status

Coverage remained the same at 95.858% when pulling a2e33fe on styd:extending-access-token-class into 96daaf5 on intridea:master.

@styd styd changed the title Allow new access token class extending AccessToken class to get access token instance of the its own class when refreshed Allow new access token class inheriting from AccessToken class to get access token instance of the its own class when refreshed May 15, 2017
@styd styd closed this Jun 16, 2017
@styd styd reopened this Jun 16, 2017
@coveralls
Copy link

coveralls commented Jun 16, 2017

Coverage Status

Coverage decreased (-0.05%) to 95.954% when pulling a2e33fe on styd:extending-access-token-class into 24acc34 on intridea:master.

3 similar comments
@coveralls
Copy link

coveralls commented Jun 16, 2017

Coverage Status

Coverage decreased (-0.05%) to 95.954% when pulling a2e33fe on styd:extending-access-token-class into 24acc34 on intridea:master.

@coveralls
Copy link

Coverage Status

Coverage decreased (-0.05%) to 95.954% when pulling a2e33fe on styd:extending-access-token-class into 24acc34 on intridea:master.

@coveralls
Copy link

Coverage Status

Coverage decreased (-0.05%) to 95.954% when pulling a2e33fe on styd:extending-access-token-class into 24acc34 on intridea:master.

@coveralls
Copy link

coveralls commented Jun 16, 2017

Coverage Status

Coverage remained the same at 96.0% when pulling d21ab69 on styd:extending-access-token-class into 24acc34 on intridea:master.

@pboling
Copy link
Member

pboling commented Oct 5, 2018

@styd If you get a chance please rebase this on master. We've improved many things in the codebase, and the CI build is now functional, and passing.

@pboling pboling added this to the 2.0.0 milestone Oct 5, 2018
@pboling pboling added the bug label Oct 5, 2018
@pboling pboling self-assigned this Oct 5, 2018
@pboling pboling self-requested a review October 5, 2018 22:20
@styd styd force-pushed the extending-access-token-class branch from d21ab69 to 9f08416 Compare October 14, 2018 02:17
@styd
Copy link
Contributor Author

styd commented Oct 14, 2018

@pboling I've rebased it on master. Please review. Thanks.

Copy link
Contributor

@meganemura meganemura left a comment

Choose a reason for hiding this comment

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

Great update. I will close #425.
This branch needs to update some points.

# @return [AccessToken] a new AccessToken
# @note options should be carried over to the new AccessToken
def refresh(params = {})
def refresh!(params = {}, access_token_opts = {}, access_token_class = self.class)
Copy link
Contributor

Choose a reason for hiding this comment

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

This must be refresh, not refresh! since it was renamed.

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Got it!

:param_name => 'o_param')
end
let(:new_access) do
NewAccessToken = Class.new(AccessToken)
Copy link
Contributor

Choose a reason for hiding this comment

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

This will be notified by RuboCop.
Use described_class instead of AccessToken.

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Thanks.

@coveralls
Copy link

coveralls commented Oct 14, 2018

Pull Request Test Coverage Report for Build 761

  • 0 of 0 changed or added relevant lines in 0 files are covered.
  • No unchanged relevant lines lost coverage.
  • Overall coverage remained the same at ?%

Totals Coverage Status
Change from base Build 714: 0.0%
Covered Lines:
Relevant Lines: 0

💛 - Coveralls

Copy link
Contributor

@meganemura meganemura left a comment

Choose a reason for hiding this comment

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

🎉

Copy link
Member

@pboling pboling left a comment

Choose a reason for hiding this comment

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

LGTM

@pboling pboling merged commit 5ea909d into ruby-oauth:master Oct 14, 2018
@pboling pboling added the in Changelog Has been added to Changelog label Mar 28, 2022
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Labels

bug in Changelog Has been added to Changelog

Projects

None yet

Development

Successfully merging this pull request may close these issues.

4 participants