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

StyleCoped scripts/models/*.cs #558

Merged
merged 14 commits into from
Aug 22, 2016

Conversation

zwrawr
Copy link
Contributor

@zwrawr zwrawr commented Aug 21, 2016

Improved Style of scripts in the models directory.

Did not rename methods/Varibles or reorder public/private

@Grenkin1988
Copy link
Contributor

You are going to have so much conflicts I think :-) You should do it in smaller parts.

@zwrawr zwrawr changed the title Stylecop models StyleCoped scripts/models/*.cs Aug 21, 2016
@svmnotn
Copy link
Contributor

svmnotn commented Aug 21, 2016

no conflicts if someone merges it right now ;)

@Grenkin1988
Copy link
Contributor

Than everyone else would have them

@bjubes
Copy link
Collaborator

bjubes commented Aug 21, 2016

The code will be clean, but everyone will hate you :)

@WardBenjamin
Copy link
Collaborator

To be honest, I'm in favor of doing this now while it's not too, too intrusive.

The only issue I see is that in a bunch of places, there's now 4 forward slashes in comments. (i.e.: ////).

@Grenkin1988
Copy link
Contributor

Grenkin1988 commented Aug 21, 2016

This is still not finished, It would be better just to force people style file when they change it.

@zwrawr
Copy link
Contributor Author

zwrawr commented Aug 21, 2016

the //// is how StyleCop wants commented out code to be

@Grenkin1988
Copy link
Contributor

There should be no commented out code #440

@zwrawr
Copy link
Contributor Author

zwrawr commented Aug 21, 2016

Before i commit that , Removing commented code will be removing commented out Debug.Log/Logger.Log is that okay?

@Grenkin1988
Copy link
Contributor

@zwrawr that people still discussing.

@zwrawr
Copy link
Contributor Author

zwrawr commented Aug 21, 2016

I made a branch off this and removed commented out code , will merge if people want that

@alexanderfast
Copy link
Collaborator

I would advise against doing this, as others have pointed out it might be problematic for other pull requests. It is much better to do cleanup in a file youre working in anyway. Preferably as a separate commit if its much, so you can look at just the diff of the other commit to see what was actually done.

If you really want to do some cleanup, then do one file at the time, try to find files that no one works in.

I appreciate the initiative though. :)

@WardBenjamin
Copy link
Collaborator

@zwrawr That's definitely an issue then. Is there a way to change that setting? Plus, it //// commented the actual comments people put there, which I don't like. I guess tbh those should be /**/ block comments, but still.

@alexanderfast
Copy link
Collaborator

There's no way to change, we can turn the rule off or leave it on. This is a good thing, cuts down on discussion. For the comments though, is it really that important? Considering what we would give up by turning if off. Everyone is used to something.

@WardBenjamin
Copy link
Collaborator

Alright, I'm okay with this now. Merging!

@WardBenjamin WardBenjamin merged commit 7bdca29 into TeamPorcupine:master Aug 22, 2016
@zwrawr zwrawr deleted the StyleCop-Models branch August 27, 2016 23:02
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.

6 participants