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.

Sign up for GitHub

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

Jump to bottom

Break meta-description ranking into function #6066

Merged
merged 3 commits into from
Oct 6, 2020
Merged

Break meta-description ranking into function #6066

merged 3 commits into from
Oct 6, 2020

Conversation

higs4281
Copy link
Member

@higs4281 higs4281 commented Oct 5, 2020 •
edited

This may not be much more efficient or elegant, but the order of preference seems clearer.

We don't lint v1 tests, but I also cleaned up some trailing-space issues in the tests.

@higs4281
Break meta-description ranking into function
8a4f55b 
This may not be much more efficient or elegant, but the order of pereference seems clearer.
@higs4281 higs4281 mentioned this pull request Oct 5, 2020
Merged
19 tasks
Scotchester
Scotchester previously requested changes Oct 6, 2020
View reviewed changes
Copy link
Contributor

@Scotchester Scotchester left a comment

Choose a reason for hiding this comment

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

I like this approach! A couple minor suggestions, but also: Any particular reason not to include search_description in the get_meta_description() method and skip the if/else in get_context()?

cfgov/v1/models/base.py Outdated Show resolved Hide resolved
cfgov/v1/models/base.py Outdated Show resolved Hide resolved
alexhoffman617
alexhoffman617 approved these changes Oct 6, 2020
View reviewed changes
Copy link
Contributor

@alexhoffman617 alexhoffman617 left a comment

Choose a reason for hiding this comment

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

Looking at this I do think its easier to understand the order of preference. I think thats worth the minimal extra loops to get all of the candidates. Merging this in and going with this method

alexhoffman617 and others added 2 commits October 6, 2020 11:46
@alexhoffman617
Update cfgov/v1/models/base.py
f4f68ae 
Co-authored-by: Scott Cranfill <scott.cranfill@cfpb.gov>
@alexhoffman617
Update cfgov/v1/models/base.py
d7b6949 
Co-authored-by: Scott Cranfill <scott.cranfill@cfpb.gov>
@alexhoffman617 alexhoffman617 merged commit 05f0acb into wagtail-search-fallback Oct 6, 2020
@alexhoffman617 alexhoffman617 deleted the meta-description-function branch October 6, 2020 15:47
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Reviewers

@alexhoffman617 alexhoffman617 alexhoffman617 approved these changes

@Scotchester Scotchester Scotchester left review comments

Assignees
No one assigned
Labels
None yet
Projects
None yet
Milestone
No milestone
Development

Successfully merging this pull request may close these issues.

None yet
3 participants
@higs4281 @Scotchester @alexhoffman617