Skip to content

Adding file standard validation in repository#2849

Merged
sokra merged 3 commits intowebpack:masterfrom
willmendesneto:add-editorconfig-validation
Sep 19, 2016
Merged

Adding file standard validation in repository#2849
sokra merged 3 commits intowebpack:masterfrom
willmendesneto:add-editorconfig-validation

Conversation

@willmendesneto
Copy link
Copy Markdown
Contributor

Please check if the PR fulfills these requirements

  • Tests for the changes have been added (for bug fixes / features)
  • Docs have been added / updated (for bug fixes / features)

What kind of change does this PR introduce? (Bug fix, feature, docs update, ...)

  • Bugfix
  • Feature
  • Code style update (formatting, local variables)
  • Refactoring (no functional changes, no api changes)
  • Build related changes
  • CI related changes
  • Other... Please describe:

What is the current behavior? (You can also link to an open issue here)
Currently someone can add files that are not respecting the .editorconfig configurations.

What is the new behavior?

The goal in this PR is add standard validation in files, based in .editorconfig default configurations. So the build should failure if someone change/add files without respect the current configuration.

Does this PR introduce a breaking change?

  • Yes
  • No

If this PR contains a breaking change, please describe the following...

This PR doesn't have breaking changes.

Other information:

@codecov-io
Copy link
Copy Markdown

codecov-io commented Aug 7, 2016

Current coverage is 91.06% (diff: 100%)

Merging #2849 into master will decrease coverage by 0.01%

@@             master      #2849   diff @@
==========================================
  Files           236        236          
  Lines          9644       9644          
  Methods        1780       1780          
  Messages          0          0          
  Branches       1936       1936          
==========================================
- Hits           8783       8782     -1   
- Misses          861        862     +1   
  Partials          0          0          

Powered by Codecov. Last update cca2cee...7e6ea22

@willmendesneto
Copy link
Copy Markdown
Contributor Author

@sokra @TheLarkInn Could you take a look and see what's going on with the coveralls, please? I think that is some unstable behaviour, because the first time was ok and right now the value was decreased, but nothing was changed in the tests.

@TheLarkInn
Copy link
Copy Markdown
Member

TheLarkInn commented Aug 7, 2016

@willmendesneto I will take a look at this locally. I don't have access to rerun the coverage reporters but I can atleast make sure everything is passing locally.

@willmendesneto
Copy link
Copy Markdown
Contributor Author

@TheLarkInn exactly. Looks weird because nothing was changed. By the way, thanks for feedback.

@TheLarkInn
Copy link
Copy Markdown
Member

No prob. And just a heads up Tobias is out for the next couple of weeks so I don't want you to feel we are ignoring PR's I'm simply more comfortable with him merging core changes even if they are pretty harmless like this one.

Comment thread .editorconfig
trim_trailing_whitespace=true No newline at end of file
[*]
indent_style = tab
indent_size = 2
Copy link
Copy Markdown
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

could you change it to 4

Copy link
Copy Markdown
Contributor Author

@willmendesneto willmendesneto Aug 17, 2016

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Currently, we have indent_size with 2 in the repository. Are we thinking in change this? Because this change has a big impact o a lot of other files.

Copy link
Copy Markdown
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

We should have tabs everywhere, so this is just a visual change. Can we omit it and let the user use his own default?

Copy link
Copy Markdown
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Sorry @sokra, but it's not clear to me. Can you elaborate your question, please?

This configuration is only for this repository, so the user that is consuming/using the package can use other configuration.

Copy link
Copy Markdown
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

My question was: What is the effect of indent_size when using indent_style = tab? Afaik it's only effect is how width the editor displays tabs. So it's only a visual effect. If this is true the next question is: Can we omit indent_size in .editorconfig so the editor (default) setting comes into effect? Why: Because every collaborator then sees his own default settings.

Copy link
Copy Markdown
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Oh, now I got it. No, it's not visual only.

What happens is that when we are using indent_style = tab with indent_size = 2 we are configuring our tab to use 2 spaces by default. So, in this case, every tab will be based in 2 spaces.

We idea is define the configuration in the repository and every collaborator should follow this configuration to avoid files with different styles/code standards. If someone tries to unfollow this default configuration, the build will breaks.

sokra added a commit that referenced this pull request Aug 17, 2016
@willmendesneto
Copy link
Copy Markdown
Contributor Author

@sokra I sent the changes and the build is broken, but I didn't change any related file. Can you check that, please?

Comment thread package.json Outdated
"travis": "npm run cover -- --report lcovonly",
"appveyor": "mocha --harmony --full-trace",
"lint": "eslint lib bin hot",
"lint-files": "npm run lint && npm run beautify-lint",
Copy link
Copy Markdown
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

spacing

@willmendesneto
Copy link
Copy Markdown
Contributor Author

I realised that this PR is broken, but there's nothing related to these changes, because I found the same unstable behaviour in other PR's that were merged into master branch, such as:

I tried to find the broken code and seems that the Travis-CI starts to break in this build.

@sokra @TheLarkInn Can you please check the master build?

@willmendesneto
Copy link
Copy Markdown
Contributor Author

Fixed. @sokra @TheLarkInn Could you take a look again, please?

@willmendesneto
Copy link
Copy Markdown
Contributor Author

@sokra @TheLarkInn Any updates in this PR? Is able to merge?

@willmendesneto
Copy link
Copy Markdown
Contributor Author

@sokra @TheLarkInn how I didn't receive any feedback I'm closing this one.

@willmendesneto willmendesneto deleted the add-editorconfig-validation branch September 17, 2016 00:31
@TheLarkInn
Copy link
Copy Markdown
Member

Sorry about this @willmendesneto we've been pretty swamped with loader API changes. We are still willing to get to this, it is just a matter of it moving kind of slow. If you are willing to reopen we will get to it

@willmendesneto willmendesneto restored the add-editorconfig-validation branch September 17, 2016 23:55
@sokra sokra merged commit 448c147 into webpack:master Sep 19, 2016
@willmendesneto willmendesneto deleted the add-editorconfig-validation branch September 20, 2016 03:32
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.

5 participants