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

pal: remove extra spaces and tabs #473

Closed
wants to merge 1 commit into from
Closed

pal: remove extra spaces and tabs #473

wants to merge 1 commit into from

Conversation

obastemur
Copy link
Collaborator

This PR cleans up the files under pal from extra spaces and tabs. In summary, this is a one time
(hopefully) mechanical change.

I’m also interested with extending CI to prevent extra spaces, tabs and bad styling.

Cleanup the files under `pal` from extra spaces. I’m also interested
with extending CI for checking the extra spaces and styling.
@digitalinfinity
Copy link
Contributor

Hi @obastemur- we discussed this internally and prefer not to take this change. Couple things:

  1. The PAL is adopted code from the CLR- we'd like to not make changes to it unless really necessary to get ChakraCore building.
  2. In the longer term, we want to move away from the PAL altogether so we'd rather not touch it for now.

Re extending CI- I'll let @dilijev speak to that 😄

@obastemur
Copy link
Collaborator Author

It's very hard to make a proper commit without editing those empty spaces. Refering to changes has nothing to do with the actual commit.

This PR doesn't not update line index etc... hence it shouldn't affect any decent diff attempt.

@obastemur obastemur closed this Mar 7, 2016
@dilijev
Copy link
Contributor

dilijev commented Mar 8, 2016

@obastemur Believe me, these kinds of changes end up sapping a lot of time unnecessarily. I have a bit of OCD about this sort of thing as well and I'm a huge champion of it, and yet I still think that this is not the right time for this change. Before going open we had a ton of this throughout the code. The script I wrote to migrate our code to the open configuration included a step to fix trailing spaces and tabs. I ended up punting on a lot of other style changes because at the time there was not a lot of agreement on that point.

More to the point, PAL is a temporary measure at the moment, and easily seeing what we've needed to change with a git diff is going to be useful. I still support this change made at the right time, and that time will be after the linux branch is stable and the dependencies are reduced as much as possible, or completely removed. Don't worry about modifying spaces in lines you're already touching, but try to avoid making changes beyond that to keep it as easy to review as possible.

Please feel free to make this change again after the Linux branch becomes stable and we make Linux builds a part of our CI. Then the effort will be mature enough to handle such a change.

As for auto code-review, see discussion in #12. To summarize, we don't want to block builds by having an extra check for style. What we would like to have instead is an auto code-review bot that will make comments about that sort of thing, and which the author has the freedom to ignore if necessary. Not all such style changes are worth making, and it's difficult to make a good logic for style that should always be applied.

For anything that would potentially be a build-breaking check (which would require almost unanimous agreement on the team), I would prioritize like this based on likelihood of consensus:

  1. Tabs
  2. Trailing whitespace
  3. Other style (very unlikely to get unanimous approval)

@dilijev
Copy link
Contributor

dilijev commented Mar 8, 2016

I had an OCD moment on tabs recently and checked myself: #425. It's not worth the time to bring in a small change like that. At some point, say around the time we implement an auto code-review system, it would be appropriate to do a pass over the whole code base and fix issues that the system would flag.

@obastemur
Copy link
Collaborator Author

@dilijev I was about to make a PR that current linux branch also builds on OSX. However the amount of empty space / tab changes are hiding the real updates on the PR. Fixing them one by one is not an option.

@digitalinfinity mentioned on gitter that there is no interest in OSX for now. Skipping both of the PRs.

IMO I don't see PAL is being replaced reasonably soon. Working on it without bringing unrelated changes is a though task..

If there is a consensus on PAL won't need much contribution, then I don't have much to say.

@dilijev
Copy link
Contributor

dilijev commented Mar 8, 2016

@digitalinfinity If @obastemur has already made changes to make a build work on OSX I don't see any reason he should hold on to them. We should just bring them in. Otherwise it would be more difficult to bring them in later. (For future work in that area, maybe he could save that effort for later.)

@obastemur Maybe you could open that OSX PR so we can see what you're talking about? I don't see why necessary changes should have anything to do with whitespace. Maybe it's not as bad as you think?

@obastemur obastemur deleted the fix_spaces branch April 30, 2016 10:50
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.

4 participants