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

Fixed coding standard violations in the Framework\Validator namespace #9251

Merged
merged 4 commits into from
Jun 13, 2017
Merged

Fixed coding standard violations in the Framework\Validator namespace #9251

merged 4 commits into from
Jun 13, 2017

Conversation

dverkade
Copy link
Member

@dverkade dverkade commented Apr 14, 2017

Fixed coding standard violations in the Framework\Validator namespace, so that it will be checked bij PHP CS and no longer be ignored while doing CI checks. Made the following changes:

  • Removed @codingStandardsIgnoreFile
  • Fixed indentation
  • Fixed number of chars per line
  • Accidentally removed new lines from the end of the file in first commit. Fixed this in the second commit.

- Removed @codingStandardsIgnoreFile
- Fixed indentation
- Fixed number of chars per line
@dverkade
Copy link
Member Author

I will look into this why the test fails, fix it and make a commit again.

@vrann vrann self-assigned this Apr 17, 2017
@vrann vrann added this to the April 2017 milestone Apr 17, 2017
@orlangur
Copy link
Contributor

Such kind of manual changes is a waste of time for both developer and PR reviewers. With ~700 files with violations and 2-4 files fixed per PR you'll need few hundreds of PRs created.

Please get familiar with approach I described in #7166 (comment) and already successfully used and fix coding standard violations in similar manner.

@vrann
Copy link
Contributor

vrann commented Apr 17, 2017

@orlangur I don't think that everything should be fixed in one pull request, more granular fixes will allow delivering changes often. However, if you'd like to work on the automated tool, please feel free to do so.

@dverkade can you please fix the issue with the static test:

FILE: ...1694-SLE1/magento2/lib/internal/Magento/Framework/Validator/Factory.php
--------------------------------------------------------------------------------
FOUND 2 ERROR(S) AFFECTING 2 LINE(S)
--------------------------------------------------------------------------------
 81 | ERROR | Opening parenthesis of a multi-line function call must be the
    |       | last content on the line
 82 | ERROR | Closing parenthesis of a multi-line function call must be on a
    |       | line by itself
--------------------------------------------------------------------------------

@vrann
Copy link
Contributor

vrann commented Apr 17, 2017

@dverkade can you please fix unit tests":

	Failed	Magento\Framework\Validator\Test\Unit\BuilderTest::testCreateValidator testCreateValidator with data set #0 History
< 1 sec
Magento\Framework\Validator\Test\Unit\BuilderTest::testCreateValidator with data set #0 (array(array('name_alias', 'Magento\\Framework\\Validator\\Test\\Unit\\Test\\StringLength', array(array(array(1, Magento\Framework\Validator\Constraint\Option))), 'name', 'property')), Magento\Framework\Validator)
Failed asserting that two objects are equal.
--- Expected
+++ Actual
@@ @@
                 '_wrappedValidator' => Magento\Framework\Validator\Test\Unit\Test\StringLength Object (...)
-                '_alias' => 'name_alias'
+                '_alias' => Array (...)
(11 more lines...)
Failed	Magento\Framework\Validator\Test\Unit\BuilderTest::testCreateValidator testCreateValidator with data set #1 History
< 1 sec
Magento\Framework\Validator\Test\Unit\BuilderTest::testCreateValidator with data set #1 (array(array('description_alias', 'Magento\\Framework\\Validator\\Test\\Unit\\Test\\StringLength', array(array(array('setMin', array(10)), array('setMax', array(1000)))), 'description', 'property')), Magento\Framework\Validator)
Failed asserting that two objects are equal.
--- Expected
+++ Actual
@@ @@
                 '_wrappedValidator' => Magento\Framework\Validator\Test\Unit\Test\StringLength Object (...)
-                '_alias' => 'description_alias'
+                '_alias' => Array (...)
(11 more lines...)
Failed	Magento\Framework\Validator\Test\Unit\BuilderTest::testCreateValidator testCreateValidator with data set #2 History
< 1 sec
Magento\Framework\Validator\Test\Unit\BuilderTest::testCreateValidator with data set #2 (array(array('sku_alias', 'Magento\\Framework\\Validator\\Test\\Unit\\Test\\StringLength', array(array(Magento\Framework\Validator\Constraint\Option\Callback)), 'sku', 'property')), Magento\Framework\Validator)
Failed asserting that two objects are equal.
--- Expected
+++ Actual
@@ @@
                 '_wrappedValidator' => Magento\Framework\Validator\Test\Unit\Test\StringLength Object (...)
-                '_alias' => 'sku_alias'
+                '_alias' => Array (...)
(11 more lines...)

@dverkade
Copy link
Member Author

dverkade commented Apr 17, 2017

@orlangur Thank you for your suggestion. Since you have a clear idea on how to fix this you can do it on all the modules in the /app/code folder. I'm focusing on the Framework. The first time it was all in one PR which became impossible to proces, so I opted for this approach where I'll fix a couple of files and do a PR per namespace. This way the PR's can be processed faster and developers can benefit from these changes immedialely while not every file is yet coding standard compliant.

@vrann I fixed the coding violations which where still in there and I fixed the failing unit tests. So this PR is now complete from my side.

@orlangur
Copy link
Contributor

@vrann @dverkade the problem is not with granularity, changes can be partial with automated approach as well, the problem is with time spent on manual changes and then rigorous review of such changes.

Ok, I'll do my best to show my approach in action asap, fixing all the codebase (modules, framework, whatever else...).

@okorshenko okorshenko modified the milestones: April 2017, May 2017 May 9, 2017
@okorshenko okorshenko modified the milestones: May 2017, June 2017 Jun 1, 2017
@okorshenko okorshenko assigned okorshenko and unassigned vrann Jun 9, 2017
@magento-team magento-team merged commit 7d4ee3e into magento:develop Jun 13, 2017
magento-team pushed a commit that referenced this pull request Jun 13, 2017
@dverkade dverkade deleted the Fixed_coding_standard_in_the_Framework-Validator branch June 22, 2018 14:55
magento-devops-reposync-svc pushed a commit that referenced this pull request Oct 8, 2024
…fix-09072024

Cia 2.4.8 beta1 develop bugfix 09072024
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