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

Add php-cs-fixer #5859

Closed
wants to merge 3 commits into from
Closed

Add php-cs-fixer #5859

wants to merge 3 commits into from

Conversation

icewind1991
Copy link
Member

@icewind1991 icewind1991 commented Jul 24, 2017

Add php-cs-fixer rules, run them and add them to cli.

This also finally formalizes the coding standard to "PSR2 with tabs and braces on the same line" which seems to be the closest to what is currently used from what I can tell.

(also secretly part of my plan to ensure that I'm the top contributor by lines of code)

@MorrisJobke
Copy link
Member

Some tests fail with "composer not found" - could you have a look?

@codecov
Copy link

codecov bot commented Jul 25, 2017

Codecov Report

Merging #5859 into master will decrease coverage by 0.33%.
The diff coverage is 62.96%.

@@             Coverage Diff              @@
##             master    #5859      +/-   ##
============================================
- Coverage     53.01%   52.67%   -0.34%     
+ Complexity    22759    22743      -16     
============================================
  Files          1404     1406       +2     
  Lines         88059    88890     +831     
  Branches       1327     1327              
============================================
+ Hits          46685    46824     +139     
- Misses        41374    42066     +692
Impacted Files Coverage Δ Complexity Δ
apps/dav/lib/CalDAV/Activity/Setting/Event.php 100% <ø> (ø) 8 <0> (ø) ⬇️
...nnector/Sabre/Exception/PasswordLoginForbidden.php 0% <ø> (ø) 2 <0> (ø) ⬇️
apps/dav/lib/CalDAV/Activity/Filter/Calendar.php 100% <ø> (ø) 7 <0> (ø) ⬇️
apps/dav/lib/Avatars/AvatarNode.php 38.46% <ø> (ø) 9 <0> (ø) ⬇️
apps/encryption/lib/Settings/Personal.php 0% <ø> (ø) 6 <0> (ø) ⬇️
...ps/encryption/lib/Migration/SetMasterKeyStatus.php 15.38% <ø> (ø) 6 <0> (ø) ⬇️
.../federation/lib/Middleware/AddServerMiddleware.php 86.66% <ø> (ø) 4 <0> (ø) ⬇️
apps/dav/appinfo/v1/webdav.php 0% <ø> (ø) 0 <0> (ø) ⬇️
apps/dav/lib/CalDAV/Activity/Filter/Todo.php 100% <ø> (ø) 7 <0> (ø) ⬇️
apps/dav/lib/Capabilities.php 40% <ø> (ø) 1 <0> (ø) ⬇️
... and 778 more

@MorrisJobke
Copy link
Member

Conflicts 🙈

@icewind1991 icewind1991 force-pushed the php-cs-fixer branch 2 times, most recently from c1aec2c to 4be09c3 Compare July 26, 2017 15:03
@MorrisJobke
Copy link
Member

Acceptance test failure:

 Scenario: viewing a favorite file in its folder does not prevent opening the details view in "All files" section # /drone/src/github.com/nextcloud/server/pull/5859/tests/acceptance/features/app-files.feature:14
150s
167
    Given I am logged in                                                                                           # LoginPageContext::iAmLoggedIn()
192s
168
    And I mark "welcome.txt" as favorite                                                                           # FilesAppContext::iMarkAsFavorite()
192s
169
    And I see that "welcome.txt" is marked as favorite                                                             # FilesAppContext::iSeeThatIsMarkedAsFavorite()
193s
170
    And I open the "Favorites" section                                                                             # AppNavigationContext::iOpenTheSection()
193s
171
    And I open the details view for "welcome.txt"                                                                  # FilesAppContext::iOpenTheDetailsViewFor()
194s
172
    And I see that the details view for "Favorites" section is open                                                # FilesAppContext::iSeeThatTheDetailsViewForSectionIsOpen()
195s
173
    And I view "welcome.txt" in folder                                                                             # FilesAppContext::iViewInFolder()
196s
174
    And I see that the current section is "All files"                                                              # AppNavigationContext::iSeeThatTheCurrentSectionIs()
196s
175
    When I open the details view for "welcome.txt"                                                                 # FilesAppContext::iOpenTheDetailsViewFor()
197s
176
      Offset within element cannot be scrolled into view: (0, 0): http://acceptance-app-files/index.php/apps/files/?dir=/&#
197s
177
      Build info: version: '2.53.1', revision: 'a36b8b1', time: '2016-06-30 17:37:03'
197s
178
      System info: host: '5fa94874b2ce', ip: '172.17.0.5', os.name: 'Linux', os.arch: 'amd64', os.version: '3.16.0-4-amd64', java.version: '1.8.0_91'
197s
179
      Driver info: driver.version: unknown (WebDriver\Exception\MoveTargetOutOfBounds)

@icewind1991 icewind1991 force-pushed the php-cs-fixer branch 2 times, most recently from 2a30d59 to b069ce6 Compare July 27, 2017 13:06
Signed-off-by: Robin Appelman <robin@icewind.nl>
@icewind1991 icewind1991 force-pushed the php-cs-fixer branch 2 times, most recently from a827037 to e09879f Compare July 28, 2017 11:10
@codecov-io
Copy link

codecov-io commented Jul 28, 2017

Codecov Report

Merging #5859 into master will decrease coverage by 0.45%.
The diff coverage is 62.31%.

@@             Coverage Diff              @@
##             master    #5859      +/-   ##
============================================
- Coverage     53.01%   52.56%   -0.46%     
+ Complexity    22759    22714      -45     
============================================
  Files          1404     1406       +2     
  Lines         88059    89369    +1310     
  Branches       1327     1327              
============================================
+ Hits          46685    46975     +290     
- Misses        41374    42394    +1020
Impacted Files Coverage Δ Complexity Δ
apps/dav/lib/CardDAV/SyncJob.php 33.33% <ø> (ø) 2 <0> (ø) ⬇️
...Connector/Sabre/Exception/UnsupportedMediaType.php 0% <ø> (ø) 1 <0> (ø) ⬇️
...deration/lib/BackgroundJob/RequestSharedSecret.php 50.74% <ø> (ø) 19 <0> (ø) ⬇️
apps/encryption/lib/HookManager.php 75% <ø> (ø) 7 <0> (ø) ⬇️
apps/dav/lib/RootCollection.php 95.65% <ø> (ø) 1 <0> (ø) ⬇️
apps/federatedfilesharing/lib/Settings/Admin.php 87.5% <ø> (ø) 4 <0> (ø) ⬇️
apps/dav/appinfo/v1/webdav.php 0% <ø> (ø) 0 <0> (ø) ⬇️
apps/dav/lib/CalDAV/Activity/Setting/Calendar.php 100% <ø> (ø) 8 <0> (ø) ⬇️
apps/encryption/lib/Session.php 100% <ø> (ø) 21 <0> (ø) ⬇️
...eratedfilesharing/lib/Settings/PersonalSection.php 0% <ø> (ø) 5 <0> (ø) ⬇️
... and 739 more

Signed-off-by: Robin Appelman <robin@icewind.nl>
Signed-off-by: Robin Appelman <robin@icewind.nl>
@icewind1991
Copy link
Member Author

All green, all rebased!

Quick @rullzer @MorrisJobke @nickvergessen review before everything starts conflicting again.

@MorrisJobke
Copy link
Member

I also did some smoketests

Copy link
Member

@ChristophWurst ChristophWurst left a comment

Choose a reason for hiding this comment

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

Nice cleanup, I like the consistency. Yet this was horrible to review 🙈

@@ -126,7 +125,7 @@ public function setName($name) {
throw new \Sabre\DAV\Exception\Forbidden();
}

list($parentPath,) = \Sabre\HTTP\URLUtil::splitPath($this->path);
list($parentPath, ) = \Sabre\HTTP\URLUtil::splitPath($this->path);
Copy link
Member

Choose a reason for hiding this comment

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

interesting formatting 🙈

$this->currentStream = $this->getStream($this->nodes[0]);

if (count(
$this->nodes
Copy link
Member

Choose a reason for hiding this comment

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

this does have to be on its own line? It's hard to read now

) > 0) {
$this->currentStream =
$this->getStream(
$this->nodes[0]
Copy link
Member

Choose a reason for hiding this comment

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

would it be possible to indent this line?

@@ -37,9 +37,11 @@
* @param int $seconds Seconds to look back at
* @return int
*/
public function getAttempts($methodIdentifier,
public function getAttempts(
$methodIdentifier,
Copy link
Member

Choose a reason for hiding this comment

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

shouldn't all parameters be indented by the same number of tabs?

Copy link
Member

Choose a reason for hiding this comment

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

(saw the same issue at other methods too)

@MorrisJobke
Copy link
Member

Conflicts \o/ - looks like it needs to be done in smaller steps. @icewind1991

$this->nodes
) > 0) {
$this->currentStream =
$this->getStream(
Copy link
Member

Choose a reason for hiding this comment

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

also I'd expect this on the line with the =

@MorrisJobke MorrisJobke added 2. developing Work in progress and removed 3. to review Waiting for reviews labels Aug 2, 2017
@rullzer
Copy link
Member

rullzer commented Aug 11, 2017

While thinking more about this I'm not a fan of this.
We already have a code review process. Sometimes things won't follow the rules we can lay out but it is better readable.

My bet is it leads to a lot of frustrated devs because they have to force push 5 times because there are tiny error according to the (limited) style rules...

@MorrisJobke
Copy link
Member

Let's not do this for now. Just too much hassle for very little advantage.

@MorrisJobke MorrisJobke deleted the php-cs-fixer branch August 14, 2017 09:28
@MorrisJobke MorrisJobke removed this from the Nextcloud 13 milestone Aug 14, 2017
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
2. developing Work in progress
Projects
None yet
Development

Successfully merging this pull request may close these issues.

None yet

6 participants