Skip to content
This repository has been archived by the owner on Jul 19, 2021. It is now read-only.

add theme-lint, remove some gems #105

Merged
merged 4 commits into from
May 17, 2017
Merged

add theme-lint, remove some gems #105

merged 4 commits into from
May 17, 2017

Conversation

NathanPJF
Copy link
Contributor

@NathanPJF NathanPJF commented Mar 17, 2017 •
edited
Loading

No immediate need to merge, but getting this started.

Why: Move theme i18n linting to the package.json so folks don't need circle Ci testing / ruby tests.

Other notes:

  • Good candidate for hooks. Run a linting task post build.

To do:

  • Make Circle Ci happy
  • Thorough check of Jekyll build for docs

@NathanPJF NathanPJF requested a review from bertiful March 17, 2017 17:27
@NathanPJF
Copy link
Contributor Author

Note: Add a command for npm run tests that will run lint and can be expanded to run more test in the future.

bertiful
bertiful approved these changes Apr 28, 2017
View reviewed changes
Copy link
Contributor

@bertiful bertiful left a comment

Choose a reason for hiding this comment

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

@NathanPJF @t-kelly mind reviewing the changes?

@bertiful
Copy link
Contributor

bertiful commented Apr 28, 2017 •
edited
Loading

Related topic Shopify/slate-tools#5. Might be better to integrate this into slate-tools directly.

@NathanPJF
Copy link
Contributor Author

@chrisberthe I'm down with the idea of integrating theme linting into one of the package. It will be linting the dist/ directory. Does slate-tools sounds like the best spot? Or should it be a core slate command?

Maybe slate-tools and it can be an optional pre-deploy / post-build task?

Let's try it out as slate-tools, and if it works out we'll modify this PR accordingly.

bertiful
bertiful suggested changes May 11, 2017
View reviewed changes
circle.yml Outdated
override:
- bundle exec rspec spec
- npm run lint
Copy link
Contributor

@bertiful bertiful May 11, 2017 •
edited
Loading

Choose a reason for hiding this comment

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

slate test

package.json Outdated
"devDependencies": {
"@shopify/slate-tools": "0.2.7",
"@shopify/theme-lint": "^1.0.2",
Copy link
Contributor

@bertiful bertiful May 11, 2017 •
edited
Loading

Choose a reason for hiding this comment

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

Can remove this and the lint script above.

@bertiful bertiful self-assigned this May 16, 2017
@bertiful bertiful requested a review from macdonaldr93 May 16, 2017 19:35
@NathanPJF
Copy link
Contributor Author

@chrisberthe want to approve your own changes? 😜

They look good to me ✅

bertiful
bertiful approved these changes May 17, 2017
View reviewed changes
Copy link
Contributor

@bertiful bertiful left a comment

Choose a reason for hiding this comment

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

Looks good 😅.

@bertiful bertiful merged commit c38be48 into master May 17, 2017
@bertiful bertiful deleted the use-themelint branch May 17, 2017 18:22
@lock
Copy link

lock bot commented Oct 26, 2018

This thread has been automatically locked since there has not been any recent activity after it was closed. Please open a new issue for related bugs.

@lock lock bot locked as resolved and limited conversation to collaborators Oct 26, 2018
Sign up for free to subscribe to this conversation on GitHub. Already have an account? Sign in.
Reviewers

@macdonaldr93 macdonaldr93 macdonaldr93 approved these changes

@bertiful bertiful bertiful approved these changes

Assignees

@bertiful bertiful

Labels
None yet
Projects
None yet
Milestone
No milestone
Development

Successfully merging this pull request may close these issues.
3 participants
@NathanPJF @bertiful @macdonaldr93