Skip to content
New issue

Have a question about this project? # for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “#”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? # to your account

Revamp search results header #462

Merged
merged 31 commits into from
Jun 18, 2020
Merged

Revamp search results header #462

merged 31 commits into from
Jun 18, 2020

Conversation

eddieantonio
Copy link
Member

@eddieantonio eddieantonio commented Jun 17, 2020

This tackles the following:


EDIT: it's ready! Here are some screenshots:

non-lemma form with definition:
Screen Shot 2020-06-18 at 11 22 42 AM

its linguistic breakdown pop-up:
Screen Shot 2020-06-18 at 11 26 13 AM

its inflectional category pop-up:
Screen Shot 2020-06-18 at 11 26 44 AM

non-lemma:
Screen Shot 2020-06-18 at 11 23 05 AM

lemma:
Screen Shot 2020-06-18 at 11 23 19 AM

@cypress
Copy link

cypress bot commented Jun 17, 2020



Test summary

75 0 6 0


Run details

Project cree-intelligent-dictionary
Status Passed
Commit 41877f0
Started Jun 18, 2020 8:27 PM
Ended Jun 18, 2020 8:30 PM
Duration 03:24 💡
OS Linux Ubuntu Linux - 16.04
Browser Electron 80

View run in Cypress Dashboard ➡️


This comment has been generated by cypress-bot as a result of this project's GitHub integration settings. You can manage this integration in this project's settings in the Cypress Dashboard

@cypress
Copy link

cypress bot commented Jun 17, 2020



Test summary

75 0 6 0


Run details

Project cree-intelligent-dictionary
Status Passed
Commit 87766e3 ℹ️
Started Jun 18, 2020 8:27 PM
Ended Jun 18, 2020 8:30 PM
Duration 02:57 💡
OS Linux Ubuntu Linux - 16.04
Browser Electron 80

View run in Cypress Dashboard ➡️


This comment has been generated by cypress-bot as a result of this project's GitHub integration settings. You can manage this integration in this project's settings in the Cypress Dashboard

@codecov-commenter
Copy link

codecov-commenter commented Jun 18, 2020

Codecov Report

Merging #462 into master will increase coverage by 0.06%.
The diff coverage is 100.00%.

Impacted file tree graph

@@            Coverage Diff             @@
##           master     #462      +/-   ##
==========================================
+ Coverage   86.38%   86.45%   +0.06%     
==========================================
  Files          67       67              
  Lines        2637     2650      +13     
==========================================
+ Hits         2278     2291      +13     
  Misses        359      359              
Impacted Files Coverage Δ
CreeDictionary/API/models.py 88.40% <100.00%> (+0.11%) ⬆️
CreeDictionary/API/schema.py 100.00% <100.00%> (ø)
...eeDictionary/API/templatetags/inflection_extras.py 64.15% <100.00%> (+5.45%) ⬆️

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 caf2bc6...41877f0. Read the comment docs.

@eddieantonio eddieantonio marked this pull request as ready for review June 18, 2020 17:21
@eddieantonio
Copy link
Member Author

@aarppe, note: once this is merged, I'd shorten the inflectional category information for N+*+D forms. They're a bit... noisy:

Screen Shot 2020-06-18 at 11 31 59 AM
Screen Shot 2020-06-18 at 11 32 21 AM

Attempts to get a description, e.g., "like: wîcihêw" or "like:
micisow" for this wordform.
Attempts to get an emoji description of the full wordclass.
e.g., "👤👵🏽" for "nôhkom"
"""
maybe_full_word_class = self.full_word_class
Copy link
Collaborator

Choose a reason for hiding this comment

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

I have some thoughts regarding this full_word_class property.

Our Wordform model has a non-standard pos field (as part of speech is depreacted), which should be replaced to a word_class field.

But if we are switching to a JIT paradigm, the way of calculating full_word_class on the fly is a good way to start.

BTW, shouldn't it be just "word_class"? The "full" is unnecessary to me.

Copy link
Member Author

Choose a reason for hiding this comment

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

Sure! I'll rename it to word_class!



# XXX: mypy can't infer this type?
PER_REQUEST_ID_COUNTER = WeakKeyDictionary() # type: WeakKeyDictionary
Copy link
Collaborator

Choose a reason for hiding this comment

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

does WeakKeyDictionary help with memory leak because the keys are garbaged collected as soon as the reference counter to the dictionary is 0?? Want some explanation.

Copy link
Member Author

Choose a reason for hiding this comment

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

Correct! When the Response object reaches a reference count of 0, the key in the dictionary is invalid, and thus, its data can be deleted



@register.filter
def unique_id(context: Any) -> str:
Copy link
Collaborator

Choose a reason for hiding this comment

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

https://docs.djangoproject.com/en/3.0/howto/custom-template-tags/

So I think "filters"( {{ ]}) takes arbitrary value, and "tags" ({% %}) are the ones that are meant to take in context? You may want to use django's @register.simple_tag(takes_context=True) here

Copy link
Member Author

Choose a reason for hiding this comment

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

I debated A LOT about this. You're right; context makes more sense, but the fact that I needed to assign and recall the ID twice makes creating a template tag... annoying... I'll give it some more thought :/

Copy link
Collaborator

@Madoshakalaka Madoshakalaka left a comment

Choose a reason for hiding this comment

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

LGTM!

# for free to join this conversation on GitHub. Already have an account? # to comment
Labels
None yet
Projects
None yet
4 participants