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

Post Normalizer: Refactor rules into individual files #5577

Merged
merged 2 commits into from
Jun 1, 2016

Conversation

blowery
Copy link
Contributor

@blowery blowery commented May 25, 2016

And make the sync rules truly sync, wrapping them in the main post normalizer.

This really just moves all of the post normalizer rules out of the one massive file into little files. This helps scoping quite a bit and makes it clearer what each rule does and what constants it depends on.

I also took the opportunity to make all of the sync rules look and feel like sync functions. No longer do they take a callback, which implies async behavior, and invoke it synchronously. There is a new wrapper that converts the sync variants to look like the old async-ish version so the refactoring doesn't impact users of the post normalizer.

To test,

  1. Run the test suite. Yay tests!
  2. Try using the Reader. Posts without a featued image spec'd should get one when a good candidate is found
  3. Try using My Posts. Same basic set of criteria.

Stuff for another PR

  • Rework lib/feed-post-store/* to use the rules directly and avoid using postNormalizer and async
  • Rework lib/posts to use the rules directly
  • Find anywhere else that's using post norm and rework it
  • Kill post norm and the async dependency

@blowery
Copy link
Contributor Author

blowery commented May 25, 2016

Tied to #5489 and #5564

@blowery blowery added [Status] Needs Review The PR is ready for review. This also triggers e2e canary tests and wp-desktop tests automatically. and removed [Status] In Progress labels May 26, 2016
@blowery
Copy link
Contributor Author

blowery commented May 26, 2016

This will make it easier to work on the normalizer, and makes each sync rule truly sync, which makes
using them from action responders much simpler.
@blowery blowery force-pushed the update/refactor-post-normalizer branch from a627398 to 0de8d2d Compare May 27, 2016 20:46
- Import find to use it
- Use new dom property to query dom
@rralian
Copy link
Contributor

rralian commented May 31, 2016

tl;dr > looks good to 🚢 to me.


The tests all pass, and I've run through the reader, post-list, and page-list pages, switched sites, and tried loading with empty localstorage and see no new issues or regressions.

I'm not totally sure how to test this bit.

  1. Try using the Reader. Posts should categorize based on content, like they do now. Posts without a featued image spec'd should get one when a good candidate is found

I can confirm that posts without a featured image are getting an image when a good candidate is found, but I'm not sure how else to test post categoziation, because I'm not familiar with the view for different types of posts. I did however check the classes applied to the article to make sure they had relevant classes added. For the most part that was correct. however I did see an example of a post that got "is-one-liner" applied when it was clearly not a one-liner. Your call whether this is an issue, but the display seems fine.

one-liner

I also noticed this issue when loading the post-list page (/posts/:site) with a clear localStorage, and I got this error. Just letting you know this appears to be an error in master and not something you should have to worry about. I'll create a separate issue to track it.

empty localstorage error

@rralian rralian added [Status] Ready to Merge and removed [Status] Needs Review The PR is ready for review. This also triggers e2e canary tests and wp-desktop tests automatically. labels May 31, 2016
@blowery blowery merged commit 9795211 into master Jun 1, 2016
@blowery blowery deleted the update/refactor-post-normalizer branch June 1, 2016 13:16
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
[Feature] Reader The reader site on Calypso. Framework Posts
Projects
None yet
Development

Successfully merging this pull request may close these issues.

3 participants