Open atanas-dev opened 6 years ago
Nice to see you here @atanas-angelov-dev!
First, it's intentional that Justin has left out text-domain. It's added when you build your theme out of Mythic.
You know how things always come up seconds after you publish something? :D I'm literally in the middle of reverting that and running i18n during the build so the PHP lint passes without having to have the textdomain in source (+ running npm build
).
Will be a couple minutes to fix and squash my changes.
@samikeijonen all fixed up - the textdomain changes have been purged.
I really like what I see here. Awesome work!
This is probably Mythic version 1.1.0 material and will need to get pulled into that when it's time. I hope this will be around the time that we can use the WPTRT Coding Standards too, which is what we've been waiting on. I'd like to have a "coding standards" type of release.
I need to educate myself a bit more on Travis CI.
I'd likely go for something that doesn't pass all the linting rules right now. For example, I want this flagged so that WordPress.org theme authors know to change it:
// @codingStandardsIgnoreLine WordPress.WP.AlternativeFunctions.file_get_contents_file_get_contents
return file_exists( $file ) ? json_decode( file_get_contents( $file ), true ) : null;
Whether Mythic itself passes any rules isn't all that important. My goal is to put theme authors in the position to make certain decisions based on the given info.
I want this flagged so that WordPress.org theme authors know to change it:
One solution I can think of is to exclude the file_get_contents
sniff entirely when building Mythic by adding an environment variable through Travis' online settings rather than the .yml. This way the build process will be successful for Mythic, but it will fail for users with projects based on Mythic since they won't have the env variable.
The downside is that if file_get_contents
ever gets used again in Mythic's repository, the Travis build will not catch it as an issue (the upside is that builds will be meaningful in PRs instead of always failing).
@justintadlock what do you think?
Sidenote:
@codingStandards*
is actually deprecated and should be replaced with phpcs:ignore
- will fix it when the rest is cleared up.
I keep PHPCS linting separate so that I can ignore warnings in Travis.
I do the same when pushing the code.
In other words, I also like to see those warnings just in case I need to look them closer when submitting to WordPress.org.
Yeah, preventing warnings from breaking builds in Travis is probably a better idea as warnings will still be visible. We can combine it with an env var from Travis' web settings instead of the .yml config so warnings still cause builds to fail for users who use mythic in their own repos (unless they define it as well of course) e.g. something like this:
- if [[ "$SNIFF" == "1" && "$SNIFF_IGNORE_WARNINGS" == "1" ]]; then vendor/bin/phpcs . --runtime-set ignore_warnings_on_exit 1; fi
- if [[ "$SNIFF" == "1" && "$SNIFF_IGNORE_WARNINGS" != "1" ]]; then vendor/bin/phpcs .; fi
Fixes #8 .
Build: https://travis-ci.org/atanas-angelov-dev/mythic/builds/447484562
Changes in this PR
.travis.yml
. This is the same file that @samikeijonen posted in #8 but with minor changes. The build consists of:npm run i18n
npm run lint
npm run build
&&
usage in npm scripts withrun-s
for cross platform support and to ensure failing lints will not prevent subsequent lints from running. The latter is useful in Travis as you can see all lint failures instead of just the ones from the first failing linter.Feedback is welcome :)