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

Fix deboosting #1318

Merged
merged 3 commits into from Jun 12, 2017
Merged

Fix deboosting #1318

merged 3 commits into from Jun 12, 2017

Conversation

psi29a
Copy link
Member

@psi29a psi29a commented Jun 11, 2017

Fixed 3 things that @scrawl mentioned here: #1314

@psi29a psi29a requested a review from a user June 11, 2017 07:33
@ghost
Copy link

ghost commented Jun 11, 2017

What's this formatting 'fix' for? Aside from being unnecessary, I think 99% of our code uses Allman style brackets not K&R style.

@psi29a
Copy link
Member Author

psi29a commented Jun 11, 2017

:(
Is this documented in our style guideline?
I'll adjust my IDE accordingly and make another commit.

@psi29a
Copy link
Member Author

psi29a commented Jun 11, 2017

@scrawl Hopefully better? fallback.cpp had some sloppy (no style) formatting that I cleaned up as well.

@ghost
Copy link

ghost commented Jun 11, 2017

No, I don't think there should be any cleanup at all. That's just asking for trouble for no reason (conflicts) if someone else wants to edit the same file.

@Allofich
Copy link
Contributor

Allofich commented Jun 11, 2017

I can understand avoiding cleanup on files that are frequently worked on, but according to fallback.cpp's history the last time it was worked on outside of psi29a's current work was over a year ago, so it doesn't really seem like a problem to me...

Edit: I can understand not wanting to set a precedent. I just don't want psi29a to feel discouraged from continuing his work.

@Allofich
Copy link
Contributor

Well, on the other hand, I guess you can never predict when someone would want to edit the file... and since it's being worked on now, it might be more likely than usual that someone would want to... Well, never mind me.

@psi29a
Copy link
Member Author

psi29a commented Jun 11, 2017

@scrawl I thought we agreed that we never 'cleanup for sake of cleaning up' unless we are editing the file in question to fix something.

In this case, fallback.cpp needed some love (Allman style like you suggested). I don't see any PR that touches the file in question so there shouldn't be any conflicts.

Would you rather I do this in another PR? (Thought this seems to go against what we talked about.)

@ghost
Copy link

ghost commented Jun 11, 2017

I think formatting should be limited to and around lines that you were already changing anyway; changing one line doesn't justify giving the whole file a makeover. Fewer changes = less risk of conflict.

I don't see any PR that touches the file in question so there shouldn't be any conflicts.

I suppose in this case (file is old and self contained) the risk seems low, but still we shouldn't go around assuming such things. Someone may be working on a PR they haven't submitted yet or have something in their stash. Then we have two forks already (tes3mp and the oblivion one) who the hell knows what they might have changed; and just because there aren't changes now doesn't mean there won't be any in the future (from users still on old code for whatever reason).
Making no exceptions seems like the best strategy to me, that way we are consistent and don't waste any time thinking about whether something should or should not be an exception.

I can understand avoiding cleanup on files that are frequently worked on, but according to fallback.cpp's history the last time it was worked on outside of psi29a's current work was over a year ago, so it doesn't really seem like a problem to me...

You could turn that around and ask, if a file is no longer worked on and forgotten then why is the 'bad' formatting even an issue?

@psi29a
Copy link
Member Author

psi29a commented Jun 11, 2017

I see your point about tes3mp, I made a revert commit. :)

@ghost
Copy link

ghost commented Jun 11, 2017

Great, now just rebase it and I'll be happy. You can merge yourself then.

…period in french localization, using std::replace is not a solution, going back to boost::lexical_cast
…rything and fixed up formatting

use Allman style brackets not K&R style, additional formatting for fallback.cpp which needed some love

revert allman formatting changes
@psi29a
Copy link
Member Author

psi29a commented Jun 11, 2017

done and done, i'll wait for last round of ci

@psi29a psi29a merged commit 830ecbe into OpenMW:master Jun 12, 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
2 participants