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 .travis.yml #61

Open
wants to merge 2 commits into
base: master
Choose a base branch
from

Conversation

atanas-dev
Copy link

@atanas-dev atanas-dev commented Oct 28, 2018

Fixes #8 .

Build: https://travis-ci.org/atanas-angelov-dev/mythic/builds/447484562

Changes in this PR

Feedback is welcome :)

@atanas-dev atanas-dev mentioned this pull request Oct 28, 2018
@samikeijonen
Copy link
Collaborator

samikeijonen commented Oct 28, 2018

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.

@atanas-dev
Copy link
Author

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.

@atanas-dev
Copy link
Author

@samikeijonen all fixed up - the textdomain changes have been purged.

@justintadlock
Copy link
Member

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.

@atanas-dev
Copy link
Author

atanas-dev commented Oct 28, 2018

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.

@samikeijonen
Copy link
Collaborator

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.

@atanas-dev
Copy link
Author

atanas-dev commented Oct 29, 2018

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
  • SNIFF defined in the .yml config (as it is now).
  • SNIFF_IGNORE_WARNINGS defined in Travis' web settings.
  • 2 separate statements since nested ifs are unreadable in a single line.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

3 participants