Skip to content

Conversation

ihor-sviziev
Copy link
Contributor

Description (*)

This PR adds .editorconfig file. It will help IDEs automatically format different types of files correctly.

As an example - right now some files in tests have incorrect line endings.

More info: https://editorconfig.org/

Fixed Issues (if relevant)

N/A

Manual testing scenarios (*)

N/A

Questions or comments

Contribution checklist (*)

  • Pull request has a meaningful description of its purpose
  • All commits are accompanied by meaningful commit messages
  • All new or changed code is covered with unit/integration tests (if applicable)
  • All automated tests passed successfully (all builds are green)

@m2-assistant
Copy link

m2-assistant bot commented Aug 16, 2019

Hi @ihor-sviziev. Thank you for your contribution
Here is some useful tips how you can test your changes using Magento test environment.
Add the comment under your pull request to deploy test or vanilla Magento instance:

  • @magento give me test instance - deploy test instance based on PR changes
  • @magento give me 2.3-develop instance - deploy vanilla Magento instance

In case you'd like to rerun tests use the following comments:

  • @magento run all tests - run all tests
  • @magento run { check name } - run a single test job (check name example: Static Tests build)

For more details, please, review the Magento Contributor Guide documentation.

@hostep
Copy link
Contributor

hostep commented Aug 16, 2019

Nice, good idea @ihor-sviziev !

I would add an exception for markdown files and set trim_trailing_whitespace = false.

Markdown syntax can have lines ending in a double space which means it should render the following line on a new line, if you don't end it with double spaces, it would be rendered on the same line.
Demo:
Screenshot 2019-08-16 at 20 02 55

Not stripping trailing whitespaces in markdown files would prevent these double spaces from getting accidentally deleted.

Not sure if there are markdown files in the repo which make use of these double spaces, but you never know...

@ihor-sviziev
Copy link
Contributor Author

ihor-sviziev commented Aug 16, 2019 via email

@hostep
Copy link
Contributor

hostep commented Aug 17, 2019

Nothing else comes to mind. And searching the web around a bit, I can't find other exceptions which would make sense for Magento code (for example a Makefile should have tabs as indentation instead of spaces, but Magento makes no use of a Makefile at this point, so this doesn't need to get added).

Would it also make sense to include a static test to enforce the rules from this file?
This one seems to work ok-ish: https://github.com/editorconfig-checker/editorconfig-checker.php
However, if I use the proposed file from this PR, and run that tool, it already reports 241.795 violations in the entire M2 codebase, so cleaning this up would take a lot of time...
Although there are also many false positives it reports, for example this indentation is being reported as incorrect (7 spaces instead of expected 4):

<tests xmlns:xsi="http://www.w3.org/2001/XMLSchema-instance"
       xsi:noNamespaceSchemaLocation="urn:magento:mftf:Test/etc/testSchema.xsd">
...

So adding a static test for these rules is probably not a good idea it doesn't seems to be "smart" enough.

Add exception for *.md files
@ihor-sviziev
Copy link
Contributor Author

Hi @hostep,
I added exception for .md files. About adding static test - I think it's not good idea for now. It's better to wait and get some feedback. That's just first version, there should be more exceptions, it have to be improved for sure.

@magento-engcom-team
Copy link
Contributor

Hi @sidolov, thank you for the review.
ENGCOM-5675 has been created to process this Pull Request
✳️ @sidolov, could you please add one of the following labels to the Pull Request?

Label Description
Auto-Tests: Covered All changes in Pull Request is covered by auto-tests
Auto-Tests: Not Covered Changes in Pull Request requires coverage by auto-tests
Auto-Tests: Not Required Changes in Pull Request does not require coverage by auto-tests

@hostep
Copy link
Contributor

hostep commented Aug 21, 2019

I have one last remark about this: suppose a developer is already using an .editorconfig file in his Magento project with different configurations than this one.
Does that mean that when that developer upgrades to - let's say Magento 2.3.4 - this file will overwrite his file? Because that might be a bit annoying.
Would it make sense to map this file to an .editorconfig.sample file in the marshalling section of the magento2-base package's composer.json file?
Something like:

        "map": [
            [
                ".editorconfig",
                ".editorconfig.sample"
            ],

Just something which popped in my mind. Not sure if this is important to consider or not?

@ihor-sviziev
Copy link
Contributor Author

ihor-sviziev commented Aug 22, 2019 via email

@magento-engcom-team magento-engcom-team merged commit 7b6ff5c into magento:2.3-develop Aug 23, 2019
@m2-assistant
Copy link

m2-assistant bot commented Aug 23, 2019

Hi @ihor-sviziev, thank you for your contribution!
Please, complete Contribution Survey, it will take less than a minute.
Your feedback will help us to improve contribution process.

@magento-engcom-team magento-engcom-team added this to the Release: 2.3.4 milestone Aug 23, 2019
@sidolov sidolov added the Auto-Tests: Not Required Changes in Pull Request does not require coverage by auto-tests label Sep 10, 2019
@ihor-sviziev ihor-sviziev deleted the ihor-sviziev-patch-1 branch January 16, 2020 11:02
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
Auto-Tests: Not Required Changes in Pull Request does not require coverage by auto-tests Progress: accept Release Line: 2.3
Projects
None yet
Development

Successfully merging this pull request may close these issues.

4 participants