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

trailing spaces cleanup #2659

Merged
merged 2 commits into from
Feb 17, 2019
Merged

trailing spaces cleanup #2659

merged 2 commits into from
Feb 17, 2019

Conversation

532910
Copy link
Contributor

@532910 532910 commented Feb 11, 2019

Fixes #2648

  • This PR is for the dev branch rather than for master.
  • This PR is compliant with the other contributing guidelines as well (if not, please describe why).
  • I have thoroughly tested my contribution.
  • The code changes are reflected in the documentation at docs/en/*.

There are a lot of trailing spaces that reduces readability and should be cleaned up.

marcelstoer
marcelstoer previously approved these changes Feb 11, 2019
@galjonsfigur
Copy link
Member

It's great PR, but I don't think removing trailing spaces in the whole project is a good idea. For example the fatfs folder, where most of the files were just copied from the upstream version - this could cause some problems if someone would want to update that code.

Also It would be great to add something like pre-commit git hook to detect trailing whitespaces - without that all this work would be worthless.

@marcelstoer
Copy link
Member

Also It would be great to add something like pre-commit git hook to detect trailing whitespaces

True, could be used for a number of other static checks as well. The major problem: hooks are local to the repository (clone) and are thus not distributed automatically. Hence, every developer would have to set up the pre-commit hook manually. GitHub does not support pre-receive hooks to do such things upstream.

@devsaurus
Copy link
Member

devsaurus commented Feb 11, 2019

where most of the files were just copied from the upstream version

I agree with @galjonsfigur that such sources should be left untouched. As consequence, this would apply to a number of sub-trees where we potentially have updates. E.g.

  • app/fatfs
  • app/lwip
  • app/mbedtls
  • app/spiffs
  • app/sqlite3

Hence, every developer would have to set up the pre-commit hook manually.

Can this be scripted - like a target in the Makefile?

@marcelstoer marcelstoer dismissed their stale review February 11, 2019 21:36

Correct to ignore copied artifacts

@marcelstoer
Copy link
Member

Hence, every developer would have to set up the pre-commit hook manually.

Can this be scripted - like a target in the Makefile?

Sure, a Git hook is just a shell script that becomes a Git hook by having a specific name and by residing in a specific folder (make could copy it there).

@TerryE
Copy link
Collaborator

TerryE commented Feb 15, 2019

See my #2648 (comment). We do need to review all of these changes and if we are going to do this, then we need a separate commit to recode any cases where trailing spaces are material first so that we can be confident by convention that we can do a bulk trailing space removal.

Also once we've done this, then adding a tab and trailing whitespace check could also be implemented.

@TerryE TerryE merged commit d77666c into nodemcu:dev Feb 17, 2019
@TerryE
Copy link
Collaborator

TerryE commented Feb 17, 2019

I did the comparison. All looks OK.

@marcelstoer marcelstoer added this to the Next release milestone Feb 17, 2019
@TerryE TerryE mentioned this pull request Feb 23, 2019
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Projects
None yet
Development

Successfully merging this pull request may close these issues.

5 participants