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

Fix plurality of mementos and resources on main replay page. Closes #534 #535

Merged
merged 5 commits into from
Aug 27, 2018

Conversation

machawk1
Copy link
Member

@ibnesayeed Kudos on the implementing the templating system, btw!

@machawk1 machawk1 requested a review from ibnesayeed August 27, 2018 16:18
@codecov
Copy link

codecov bot commented Aug 27, 2018

Codecov Report

Merging #535 into master will decrease coverage by 0.91%.
The diff coverage is 50%.

Impacted file tree graph

@@            Coverage Diff            @@
##           master    #535      +/-   ##
=========================================
- Coverage   24.41%   23.5%   -0.92%     
=========================================
  Files           6       6              
  Lines        1151    1353     +202     
  Branches      170     260      +90     
=========================================
+ Hits          281     318      +37     
- Misses        853    1010     +157     
- Partials       17      25       +8
Impacted Files Coverage Δ
ipwb/replay.py 15.3% <50%> (+0.94%) ⬆️

Continue to review full report at Codecov.

Legend - Click here to learn more
Δ = absolute <relative> (impact), ø = not affected, ? = missing data
Powered by Codecov. Last update f21dc78...e2cfaaa. Read the comment docs.

Copy link
Member

@ibnesayeed ibnesayeed left a comment

Choose a reason for hiding this comment

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

Rather than manually pluralizing and passing too many variables, I would encourage using inflect library and making the function available as a filter in the template by utilizing Context Processors.

@machawk1
Copy link
Member Author

@ibnesayeed That would be a good improvement but for now, this fixes the issue described in #534.

@machawk1
Copy link
Member Author

inflect is also GPLv3, @ibnesayeed (related: #382)

@ibnesayeed
Copy link
Member

I will leave the decision of merging this PR in its current state or otherwise to you discretion because I am not a big fan of hacky approaches on every step that pile up and make maintenance difficult (and invite more hacks for every new change).

@ibnesayeed
Copy link
Member

inflect is also GPLv3

We can avoid using that, but still move the pluralization logic into Context Processors.

@machawk1
Copy link
Member Author

@ibnesayeed Ok, I'll get a little more familiar with Flask's context processors and implement that with this same PR.

@machawk1 machawk1 changed the title Fix plurality of mementos and resources on main replay page. Closes #534 [WIP] Fix plurality of mementos and resources on main replay page. Closes #534 Aug 27, 2018
@machawk1
Copy link
Member Author

@ibnesayeed I updated this to use a context processor. Please have another look.

@machawk1
Copy link
Member Author

./ipwb/replay.py:73:5: E731 do not assign a lambda expression, use a def needs to be fixed before we merge this.

@machawk1 machawk1 merged commit 4d09ed3 into master Aug 27, 2018
@machawk1 machawk1 deleted the issue-534 branch August 27, 2018 18:48
@ibnesayeed ibnesayeed changed the title [WIP] Fix plurality of mementos and resources on main replay page. Closes #534 Fix plurality of mementos and resources on main replay page. Closes #534 Aug 27, 2018
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.

2 participants