Skip to content
This repository has been archived by the owner on Nov 5, 2024. It is now read-only.

Update code to comply with WP Coding Standards (rebased) #32

Closed
wants to merge 94 commits into from

Conversation

Rumperuu
Copy link
Collaborator

@Rumperuu Rumperuu commented Feb 21, 2021

Updates the codebase to ensure compliance with the WP Coding Standards.

Also Composerises the project to allow for the installation of a pre-commit PHP_CodeSniffer hook (and other hooks later on).

Note 1: because pretty much every line of every file is being changed (even if just to change indentation format), the diff tool is not particularly useful on this PR.

Note 2: This branch's commit history will be squashed on merge, so do not make any commits to this branch that you're going to want to remain in the project history. Keep it scoped to WP Coding Standards compliance only.

This is a duplicate of #23, except rebased to separate commit 3a44a19 into two different commits (27abf83 and b349544), ensuring the files in class/dashboard/* are properly tracked as having been renamed to class/layout/*, rather than just deleted.

Replace #23

Close #8
Close #10
Close #11

Rumperuu and others added 30 commits February 20, 2021 20:00
Reordered entries in Contributors field of readme.txt in sync with project.
Correct my typo: missing comma.
Change misleading variable names C_BOOL_… to C_STR_… — The type in the variable name was useful to show the intention.
Change misleading variable names C_BOOL_… to C_STR_… — The type in the variable name was useful to show the intention.
Change misleading variable names C_BOOL_… to C_STR_… — The type in the variable name was useful to show the intention.
Change misleading variable names C_BOOL_… to C_STR_… — The type in the variable name was useful to show the intention.
Deferring jQuery Tools to the footer breaks jQuery tooltip display.
Fix docblock.
(Trailing comma after last argument can stay.)
class/init.php Outdated Show resolved Hide resolved
class/init.php Outdated
*/
wp_enqueue_script(
'mci-footnotes-jquery-tools',
plugins_url( 'footnotes/js/jquery.tools.min.js' ),
array(),
'1.2.7.redacted.2'
'1.2.7.redacted.2',
true
Copy link
Collaborator Author

Choose a reason for hiding this comment

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

This should be false, fixed by @pewgeuges in 9226a40

These are class variables because they’re used both in search() and in reference_container(), hence the TODO item has been replaced with an explanatory comment.
The deleted attribution is now restored.
The version number for the variables is corrected (2.3.0, not 2.0.0).
Docblock heading corrected to “The slug and identifier separator.”
@markcheret markcheret self-requested a review February 23, 2021 13:14
Copy link
Owner

@markcheret markcheret left a comment

Choose a reason for hiding this comment

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

This is only a test comment. Thanks for your work

@markcheret
Copy link
Owner

This is only a test comment. Thanks for your work

another test

class/init.php Outdated Show resolved Hide resolved
class/task.php Show resolved Hide resolved
@Rumperuu
Copy link
Collaborator Author

Rumperuu commented Feb 23, 2021

This GitHub thing is totally unusable on this branch. The plethora of mostly duplicated commits due to bugs on the platform is hard to review in detail, while the changed files view is worthless. It just shows all files with changes in them, and is unable to detect the many identical strings. @Rumperuu did not move blocks of code, so the useless diff view is the sole responsibility of the platform. Note that all space indentation was already converted back to tabs! So the code is fully diffable, but the platform is failing.

As we must track deleted docblocks and disruptive changes to the code, I cannot approve until I’ve diffed all changed files locally in VSCode.

That will take some time, and over this we should add all the missing docblocks as well, since we’re on it and so much is already fixed (or broken).

You're right, the indentation wasn't the issue — it was the newlines. The files in main don't have any (the ^M characters in this git diff output):

rumperuu@ben-desktop:footnotes$ git diff -R main includes.php
diff --git b/includes.php a/includes.php
index 9501097..aa35d1c 100644
--- b/includes.php
+++ a/includes.php
@@ -1,39 +1,37 @@
-<?php
-/**
- * Includes all common files.
- *
[shortened]
+<?php^M
+/**^M
+ * Includes all common files.^M
+ *^M

I assume this comes from copy-pasting code into the Web tool — I didn't notice it until now because all my earlier PRs (e.g., adding the README were a) operating on entirely new files and b) done through the GitHub Web tool.

Solution: I'll checkout main, renormalise all of the files so that every line has a line ending char and then make a separate PR (that will only add line endings). Once that's merged with main, the diff tool will no longer pick up on the differences in line endings!

@Rumperuu Rumperuu mentioned this pull request Feb 23, 2021
@markcheret
Copy link
Owner

Do you need to rebase again?

@Rumperuu
Copy link
Collaborator Author

Do you need to rebase again?

It's working locally now:

rumperuu@ben-desktop:footnotes$ git diff main includes.php
diff --git a/includes.php b/includes.php
index 732cfb9..9501097 100644
--- a/includes.php
+++ b/includes.php
@@ -3,36 +3,37 @@
  * Includes all common files.
  *
  * @filesource
- * @author Stefan Herndler
+ * @package footnotes
  * @since 1.5.0 14.09.14 13:40

I think the GitHub one might have some caching so it might take a while to update.

I have only been dealing with PHP files in this branch so far, so the non-PHP files
still lack line endings. This commit adds line endings to only those files, which
should ensure that they are in line with the files on `main` and do not appear in
this PR's diff view.
@Rumperuu
Copy link
Collaborator Author

Do you need to rebase again?

It's working locally now:

rumperuu@ben-desktop:footnotes$ git diff main includes.php
diff --git a/includes.php b/includes.php
index 732cfb9..9501097 100644
--- a/includes.php
+++ b/includes.php
@@ -3,36 +3,37 @@
  * Includes all common files.
  *
  * @filesource
- * @author Stefan Herndler
+ * @package footnotes
  * @since 1.5.0 14.09.14 13:40

I think the GitHub one might have some caching so it might take a while to update.

Not a caching issue; I think it's still diffing this branch with the version of main that it was branched from rather than the current version (with the line endings), so rather than mess around trying to rebase again I've just recreated the branch and created a new PR at #38

Closing this PR

@Rumperuu Rumperuu closed this Feb 23, 2021
@Rumperuu Rumperuu deleted the wp-coding-standard--rebased branch February 23, 2021 16:56
@pewgeuges pewgeuges added the wpcs-compliance WordPress Coding Standards conformance label Mar 4, 2021
Sign up for free to subscribe to this conversation on GitHub. Already have an account? Sign in.
Labels
documentation Improvements or additions to documentation duplicate This issue or pull request already exists security Anything with security implications wpcs-compliance WordPress Coding Standards conformance
Projects
None yet
3 participants