Skip to content
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

Remove destructive assignment for repository #876

Merged
merged 2 commits into from
Jul 13, 2017

Conversation

pocke
Copy link
Contributor

@pocke pocke commented Mar 23, 2017

Octokit::Repository breaks an argument if the argument is a Hash.
This change fixes the problem.

Reproduce

require 'octokit'

repo = {name: 'foo', username: 'bar'}

Octokit::Repository.new(repo)

p repo # => {:name=>"foo", :username=>"bar", :repo=>"foo", :user=>"bar", :owner=>"bar"}

@coveralls
Copy link

Coverage Status

Coverage increased (+0.05%) to 99.309% when pulling 7eeed08 on pocke:remove-destructive-assignment into d9b2ed7 on octokit:master.

1 similar comment
@coveralls
Copy link

Coverage Status

Coverage increased (+0.05%) to 99.309% when pulling 7eeed08 on pocke:remove-destructive-assignment into d9b2ed7 on octokit:master.

pocke added a commit to pocke/octokit.rb that referenced this pull request Mar 23, 2017
…s failed

Note: Test cases for this PR is failed now. It will be fixed if octokit#876 was merged.

Currently, an error message of `Octokit::Repository.new` is unkind.

For example:

```ruby
Octokit::Repository.new(nil) # => Octokit::InvalidRepository: Invalid Repository. Use user/repo format.
```

If an user want to pass an id(Integer), this message is wrong for the user.

This change improves the error message.
So, this change makes debugging easier.
@pocke
Copy link
Contributor Author

pocke commented Apr 6, 2017

ping

@kytrinyx
Copy link
Contributor

I think this is a very reasonable change.

Some people may be relying on the mutation, which I would consider a bug. My suggestion for this would be to do a patch release with this change.

@coveralls
Copy link

Coverage Status

Coverage increased (+0.0006%) to 99.276% when pulling d46f638 on pocke:remove-destructive-assignment into ad74f84 on octokit:master.

3 similar comments
@coveralls
Copy link

Coverage Status

Coverage increased (+0.0006%) to 99.276% when pulling d46f638 on pocke:remove-destructive-assignment into ad74f84 on octokit:master.

@coveralls
Copy link

Coverage Status

Coverage increased (+0.0006%) to 99.276% when pulling d46f638 on pocke:remove-destructive-assignment into ad74f84 on octokit:master.

@coveralls
Copy link

Coverage Status

Coverage increased (+0.0006%) to 99.276% when pulling d46f638 on pocke:remove-destructive-assignment into ad74f84 on octokit:master.

@tarebyte tarebyte merged commit 94c41a7 into octokit:master Jul 13, 2017
@pocke pocke deleted the remove-destructive-assignment branch July 14, 2017 00:08
pocke added a commit to pocke/octokit.rb that referenced this pull request Jul 14, 2017
…s failed

Note: Test cases for this PR is failed now. It will be fixed if octokit#876 was merged.

Currently, an error message of `Octokit::Repository.new` is unkind.

For example:

```ruby
Octokit::Repository.new(nil) # => Octokit::InvalidRepository: Invalid Repository. Use user/repo format.
```

If an user want to pass an id(Integer), this message is wrong for the user.

This change improves the error message.
So, this change makes debugging easier.
@kytrinyx kytrinyx mentioned this pull request Dec 20, 2017
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

4 participants