Coding standard compliance

backdrop-contrib / i18n

Collection of modules to extend Backdrop CMS multilingual capabilities

https://backdropcms.org/project/i18n

GNU General Public License v2.0

2 stars 5 forks source link

Coding standard compliance #23

Closed indigoxela closed 4 years ago

indigoxela commented 4 years ago

Currently there are a lot of coding standard violations in many files.

Pretty code is readable and easier to maintain code. So this will be my next step.

indigoxela commented 4 years ago

Done. It's impossible to fully comply the standards, though, for instance we can't simply rename existing classes and methods (camelcase).

indigoxela commented 4 years ago

Reopening this as @serundeputy was so kind to provide a .travis.yml config in a new PR:

https://github.com/backdrop-contrib/i18n/pull/25

serundeputy commented 4 years ago

Done. It's impossible to fully comply the standards, though, for instance we can't simply rename existing classes and methods (camelcase).

for these cases we can/should add

// @TODO: Rename classes to CamelCase in 2.x.
// @codingStandardsIgnoreLine

indigoxela commented 4 years ago

Regarding @codingStandardsIgnoreLine: we'd need a lot of those, so I'm undecided yet.

Newer phpcs versions would allow to --exclude sniffs via command line, but backdrop/coder requires old 1.5. Ever thought about upgrading? I'm aware this requires rewriting sniffs.

serundeputy commented 4 years ago

I think about upgrading ... I originally pinned that version to support php 5.3, but these days i'm less concerned about that, but worth noting that backdrop core still supports back to php5.3.

serundeputy commented 4 years ago

The travis builds/tests are not running. I'd recommend we go through and file PRs per directory. ~~i18n_forum; PR https://github.com/backdrop-contrib/i18n/pull/26~~

indigoxela commented 4 years ago

Nope, we have submodules, that should get completely ignored.

See https://github.com/backdrop-contrib/i18n/issues/1 for details.

In other words: don't waste time on something that we don't actually need. :wink:

Submodules to ignore:

i18n_block
i18n_forum
i18n_select
i18n_user
i18n_variable

The reason why they're still there is to figure out a migration path from D7.

serundeputy commented 4 years ago

ahh; ok; let me see about telling travis not to check those

serundeputy commented 4 years ago

ok; assuming these are legit failures: https://travis-ci.org/github/backdrop-contrib/i18n/builds/679360294

indigoxela commented 4 years ago

All those missing parameter types will be a bunch of work - it's sometimes hard to tell what the right type is in those nested functions... especially i18n_string is pretty complex.

indigoxela commented 4 years ago

I've left comments on your current PR.

A quick idea: instead of blocking phpcs on files containing classes with underscores even for local phpcs runs (via @codingStandardsIgnoreFile), we could exclude them on the command line run in travisCI.

That way people don't get a test failure on things no one can fix in 1.x, but it's still possible to keep an eye on other violations when run locally.

@serundeputy what do you think?

Affected files:

i18n_object.inc
i18n_translation/i18n_translation.inc
i18n_taxonomy/i18n_taxonomy.inc
i18n_path/i18n_path.inc
i18n_string/i18n_string.inc
i18n_menu/i18n_menu.inc
i18n_field/i18n_field.inc

serundeputy commented 4 years ago

That's a great idea!

indigoxela commented 4 years ago

Although coding standards will be an ongoing task, I'm closing this issue for now, as travisCI test is green.