Skip to content

Format and White-spacing #205

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

Merged
merged 5 commits into from
Apr 18, 2016
Merged

Format and White-spacing #205

merged 5 commits into from
Apr 18, 2016

Conversation

vmuriart
Copy link
Contributor

To address #184.
Settled on using VS standard formatting for .cs and Python pep8 for .py.
Did some house cleaning and removed/integrated old docs, solution files, etc.

Should hopefully help solve constant whitespacing issue on pull requests.

Note, other than white-spacing and formatting nothing in the code has been changed.

@vmuriart
Copy link
Contributor Author

@tonyroberts any idea why Appveyor isn't running anymore on the pull request? Noticed it yesterday. I verified the build against my Appveyor account and its passing, so it Travis-ci

@vmuriart
Copy link
Contributor Author

As I side note, i don't fully agree with VS's choice of style, but I'd rather see consistent formatting.

@tonyroberts
Copy link
Contributor

I'm fine with cleaning up whitespace etc, but I think reorganising the file structure should be done in a different PR. In particular, I don't think moving very old and out dated documentation into the root folder is a good idea. I think it would be better if it was updated and then moved - rather than delay these whitespace changes until that's done, dropping them from this PR seems sensible.

@tonyroberts
Copy link
Contributor

Also the wording of commit f72c7e0 comes across as a bit derogatory towards mono (even if that wasn't the intention) and could give someone casually looking through the commit history the impression that mono is less supported than the microsoft runtime, which is not the case.

@tonyroberts
Copy link
Contributor

@vmuriart hmm not sure what happened to the appveyor CI - looks like there was some problem with the github settings, which is odd as I'm pretty sure I didn't change them! Anyway, hopefully it will be working again now..

@vmuriart vmuriart force-pushed the master branch 2 times, most recently from b67d068 to ffe376b Compare April 18, 2016 11:50
@vmuriart
Copy link
Contributor Author

@tonyroberts I was having similar thoughts about the file cleanup after i submitted the pr. Just rebased and removed them from this latest one, I'll submit a separate one for those.

Good catch on the mono commit. I meant that in a joking manner (monkey-droppings) with the intention of removing their old format files since they now support the same files as vs.

Looks like travis is still the only one running, atleast on this PR. I had a similar issue on my fork though (Travis being the problem in my case). To fix it I had to remove the hook for it, and re-add it from github.

@vmuriart
Copy link
Contributor Author

@tonyroberts what are your thoughts on git fast-forward for merges? I personally use a combination of both git ff and merge-commits depending on the change. Simple changes (1 or 2 small commits) I fastforward, and more complex ones get merge-committed.

@tonyroberts
Copy link
Contributor

@vmuriart cool, I'll merge this now and take a look at why the commit hooks aren't working later. I'll try just recreating it as you suggest. Thanks for doing this!

@tonyroberts
Copy link
Contributor

tonyroberts commented Apr 18, 2016

@vmuriart wrt ff, it really depends on the situation. For simple changes I prefer to ff where possible, but when adding significant new feature or a larger set of changes I prefer to use a feature branch and keep the merge commit - so sounds like we're thinking along the same lines :)

@tonyroberts tonyroberts merged commit 6139989 into pythonnet:master Apr 18, 2016
@den-run-ai
Copy link
Contributor

@vmuriart @tonyroberts the indent(ation) style for curly braces is a mess even after merge. Here are 2 contradictory examples:

Allman style:
https://github.com/pythonnet/pythonnet/pull/205/files#diff-1b72bb3fa8d022b65bf52c6d695a6562R44

K&R style:
https://github.com/pythonnet/pythonnet/pull/205/files#diff-0ac34f0651a9febc4d8713852516ea6cR50

@vmuriart
Copy link
Contributor Author

Good catch. Not sure why the linter's didn't pick it up.

They should all be defaulting the K&R style since that's VS's default.

On Mon, Apr 18, 2016 at 10:50 AM, denfromufa notifications@github.com
wrote:

@vmuriart https://github.com/vmuriart @tonyroberts
https://github.com/tonyroberts the indent(ation) style for curly braces
is a mess even after merge. Here are 2 contradictory examples:

Allman style:

https://github.com/pythonnet/pythonnet/pull/205/files#diff-1b72bb3fa8d022b65bf52c6d695a6562R44

K&R style:

https://github.com/pythonnet/pythonnet/pull/205/files#diff-0ac34f0651a9febc4d8713852516ea6cR50


You are receiving this because you were mentioned.
Reply to this email directly or view it on GitHub
#205 (comment)

@vmuriart
Copy link
Contributor Author

Ah, just realized whats going on.
I only corrected the formatting on the .cs and .py files for bracketing
and such.
The whitespacing was done to all files. The file you pointed out as being
inconsistent was a .c file that only got the trailing-whitespace
treatment.

On Mon, Apr 18, 2016 at 11:32 AM, Victor Uriarte vmuriart@gmail.com wrote:

Good catch. Not sure why the linter's didn't pick it up.

They should all be defaulting the K&R style since that's VS's default.

On Mon, Apr 18, 2016 at 10:50 AM, denfromufa notifications@github.com
wrote:

@vmuriart https://github.com/vmuriart @tonyroberts
https://github.com/tonyroberts the indent(ation) style for curly
braces is a mess even after merge. Here are 2 contradictory examples:

Allman style:

https://github.com/pythonnet/pythonnet/pull/205/files#diff-1b72bb3fa8d022b65bf52c6d695a6562R44

K&R style:

https://github.com/pythonnet/pythonnet/pull/205/files#diff-0ac34f0651a9febc4d8713852516ea6cR50


You are receiving this because you were mentioned.
Reply to this email directly or view it on GitHub
#205 (comment)

@vmuriart
Copy link
Contributor Author

Didn't realize that there were only 2 .c files. Had i known I wouldn't have skipped them...

@den-run-ai
Copy link
Contributor

@vmuriart @tonyroberts I think this PR should include one editconfig settings file for other IDEs such as Atom, Sublime, VS Code, MonoDevelop, NPD++. See example here:

https://github.com/OmniSharp/omnisharp-roslyn/blob/dev/.editorconfig

@vmuriart
Copy link
Contributor Author

I forgot to add the .editorconfig but I agree. I can include one with the pr to remove some of the old files

@vmuriart vmuriart mentioned this pull request Jan 7, 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.

3 participants