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鈥檒l occasionally send you account related emails.

Already on GitHub? Sign in to your account

Revert Nokogiri refactor #11

Merged
merged 4 commits into from Oct 26, 2017
Merged

Revert Nokogiri refactor #11

merged 4 commits into from Oct 26, 2017

Conversation

jeremyhansonfinger
Copy link

@jeremyhansonfinger jeremyhansonfinger commented Oct 25, 2017

Problem

The attempts to refactor this app to use Nokogiri and implement content style linting were never entirely successful. Rather than reverting at the time (a year ago! 馃槺 ) we just changed Policial to use the last known good revision (e406c19). But Justin never came back and Nathan Marks (who attempted to fix it) also left. So we're left with an app where HEAD is broken and Policial depends on revision four commits back.

Look at discussion in PRs #6, #8, #9, and #10 for additional history.

Solution

As per https://github.com/Shopify/shopify/pull/135318#issuecomment-339423380, let's revert all these changes so that HEAD is the last known good state. I've maintained the original commits in a branch called nokogiri-refactor.

FYI @EiNSTeiN-

@nwtn
Copy link

nwtn commented Oct 25, 2017

This is just a straight revert, right? I haven鈥檛 checked the code closely, but overall LGTM. I 馃帺 with https://github.com/Shopify/shopify/pull/135318 and it works fine, but the ContentStyle checks aren鈥檛 happening anymore. Is that expected?

@jeremyhansonfinger
Copy link
Author

jeremyhansonfinger commented Oct 25, 2017

Yup, content style shouldn't be running. There's no config for it in master: https://github.com/Shopify/shopify/blob/master/config/erb-lint.yml

And yes, I created this with git revert <SHA> <SHA> etc.

@jeremyhansonfinger
Copy link
Author

image

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.

None yet

3 participants