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

Dev #54

Merged
merged 43 commits into from
Nov 6, 2023
Merged

Dev #54

merged 43 commits into from
Nov 6, 2023

Conversation

yigithanbalci
Copy link
Contributor

Copy link
Owner

@KnutZuidema KnutZuidema left a comment

Choose a reason for hiding this comment

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

Thanks a lot for the contribution. Please have a look at my comments.

riot/lol/constants.go Outdated Show resolved Hide resolved
riot/val/match_test.go Show resolved Hide resolved
riot/val/model.go Outdated Show resolved Hide resolved
riot/client.go Outdated Show resolved Hide resolved
riot/lol/challenges.go Outdated Show resolved Hide resolved
riot/lol/challenges.go Outdated Show resolved Hide resolved
riot/lol/challenges.go Outdated Show resolved Hide resolved
riot/lol/challenges.go Outdated Show resolved Hide resolved
riot/val/constants.go Outdated Show resolved Hide resolved
riot/val/constants.go Outdated Show resolved Hide resolved
@yigithanbalci
Copy link
Contributor Author

I have updated it according to your review. But I couldn't figure out why the pipeline is lingering.

Copy link
Owner

@KnutZuidema KnutZuidema left a comment

Choose a reason for hiding this comment

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

Please also check linter issues raised.

README.md Outdated Show resolved Hide resolved
@codecov
Copy link

codecov bot commented Oct 27, 2023

Codecov Report

Merging #54 (a880405) into master (fe6add7) will not change coverage.
The diff coverage is 100.00%.

@@            Coverage Diff             @@
##            master       #54    +/-   ##
==========================================
  Coverage   100.00%   100.00%            
==========================================
  Files           21        27     +6     
  Lines          817       955   +138     
==========================================
+ Hits           817       955   +138     
Files Coverage Δ
golio.go 100.00% <ø> (ø)
internal/client.go 100.00% <100.00%> (ø)
riot/lol/challenges.go 100.00% <100.00%> (ø)
riot/lol/client.go 100.00% <100.00%> (ø)
riot/lol/match.go 100.00% <100.00%> (ø)
riot/lol/model.go 100.00% <100.00%> (ø)
riot/lol/third_party_code.go 100.00% <100.00%> (ø)
riot/lol/tournament.go 100.00% <100.00%> (ø)
riot/val/client.go 100.00% <100.00%> (ø)
riot/val/content.go 100.00% <100.00%> (ø)
... and 3 more

📣 We’re building smart automated test selection to slash your CI/CD build times. Learn more

@yigithanbalci
Copy link
Contributor Author

Not all linting problems were showing at once, therefore I have run CI in my forked branch and merged changes to my dev branch which was the merging branch for this PR. I guess this time it should pass all controls in the pipeline

.golangci.yml Outdated Show resolved Hide resolved
codecov.yml Outdated Show resolved Hide resolved
}

// NewClient returns a new instance of a League of Legends client.
func NewClient(base *internal.Client) *Client {
Copy link
Owner

Choose a reason for hiding this comment

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

this constructor is not covered by the test suite. add a test for this constructor.

@KnutZuidema KnutZuidema merged commit 53eab1e into KnutZuidema:master Nov 6, 2023
@KnutZuidema
Copy link
Owner

Thanks a ton @yigithanbalci for your work. 🙏

This was referenced Nov 7, 2023
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.

2 participants