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

Improve specs and fix some Credo warnings #5

Closed
wants to merge 6 commits into from
Closed

Improve specs and fix some Credo warnings #5

wants to merge 6 commits into from

Conversation

rustra
Copy link
Contributor

@rustra rustra commented Jun 19, 2020

mix dialyzer still has 2 warnings, would be great to fix them as well.

@marcelotto
Copy link
Member

Sorry, but this PR is very hard to review, not only because of its size, but also because the amount of changes in the files exceeds GitHub's ability to present the diff in an easy to review form.

Regarding Credo, I don’t take it too seriously. I just added it in the beginning to see if it reveals something meaningful, but didn’t find it that helpful. From glancing over your styling changes, I agree with most of what I’ve seen, but again the amount is too much. It would have helped if you had split up your changes into separate commits, so they could have been reviewed one by one. Also, please follow the contribution guidelines and create a branch for your pull request. GitHub doesn't seem to be able to show the separate commits otherwise on the PR.

In order to avoid any arguments over style and make the diffs simpler by allowing them to focus on real changes, I'll apply the mix formatter. I will then gratefully accept your typespec contributions and any other improvements of the code, if I'm able to review them.

@rustra
Copy link
Contributor Author

rustra commented Jun 20, 2020

Nice to see that you've applied mix formatter that makes to track diff easier for this PR.
However I've tried to push separate branch to this Github repo and had notification that I don't have access rights for that. Could you please check if it really allowed?

@marcelotto
Copy link
Member

You have to create the branch on your fork.

@rustra rustra closed this Jun 20, 2020
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.

None yet

2 participants