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

Adding missing documentation and fixing style issues. #43

Merged
merged 4 commits into from
Dec 18, 2016

Conversation

olivif
Copy link
Contributor

@olivif olivif commented Dec 17, 2016

I noticed the project had 124 stylecop warnings, so I fixed a few (down to 70 after this PR). Would be great to fix all and set warnings as errors on the project to get build time protection on this.

@dlemstra
Copy link
Member

Thanks for helping us out with fixing those warnings. I noticed that some of your params/returns don't start with a capital letter. I know it is a bit OCD but could you change that?

@olivif
Copy link
Contributor Author

olivif commented Dec 17, 2016

thanks @dlemstra. fixed the uppercase issue now. I also looked into if this can be a stylecop rule, so we can automate it, unfortunately couldn't find anything 😞

@dlemstra
Copy link
Member

There is a StyleCop rule for this but it has not yet been implemented. I am waiting on my pull request (DotNetAnalyzers/StyleCopAnalyzers#2242) being accepted and then can we enable the rule in this project.

@olivif
Copy link
Contributor Author

olivif commented Dec 17, 2016

@dlemstra awesome!

@JimBobSquarePants
Copy link
Member

Ah this is great! I'm trying to make the code as readable as possible so this really helps. 👍

@JimBobSquarePants JimBobSquarePants merged commit 25ec9ce into SixLabors:master Dec 18, 2016
@olivif
Copy link
Contributor Author

olivif commented Dec 18, 2016

Thanks for merging! Yes, I have some more ideas on how to improve it, just wanted to start with a small-ish PR to kick things off 😄

@JimBobSquarePants
Copy link
Member

That's great to hear! Thanks for helping out 😄

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.

3 participants