-
-
Notifications
You must be signed in to change notification settings - Fork 11
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
feat(templates): Enhance I'm Now Following [#332] #530
feat(templates): Enhance I'm Now Following [#332] #530
Conversation
3bbb1c6
to
291e427
Compare
That sounds great, @TheCleric. Let us know when this is ready for review and we'll dig in. Totally makes sense about the template question. The devil will be in the details, but I follow your logic! |
Okay I've finished testing and this seems good to go for me with one big caveat: I don't think I can do the Twitter API without paying money, so that would need someone else to validate. |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Just one comment for me. I leave the full review to you @ERosendo.
@ERosendo thanks for the feedback. I think I've addressed what you pointed out. |
The code looks good and the templates seem well-structured. @mlissner, if you're happy with the content of the templates, I believe we can merge this PR. Here are screenshots showcasing the different templates:
|
Looks awesome! This is a huge improvement, thank you for the review Eduardo, and thank you for the great work, @TheCleric. So cool! |
This should implement #332
Things I definitely want others' input on:
is_bankruptcy
, which I went with what SEEMED correct, but I could be very wrong (see comment in that function).test_tasks.py
file. Not sure if that would be good as well, or would overlap a ton with the tests intest_templates.py
that I added.For the template code, as I was adding the desired feature, I experimented with a few different ways of doing it. I first attempted to continue our current path of 1 template per option. Since before we only had one option, that was fine since that only meant two templates. However I discovered this grows exponentially. 2 options means 4 templates (e.g., post, post w/ article, post w/ date, post w/ article + date). And 3 options means 8 templates.
This was starting to get unwieldy (especially for the code for deciding which template to use). I also thought about adding something like sub templates where you could inject templates into other templates for some of this conditional stuff. This just felt like writing my own templating engine (poorly). My next experiment was trying to do a lazy loaded f string so I could use f string conditionals in the templates. This proved to be a bit unwieldy as well since to do so I would either have to make the template strings into lambda functions (which seemed funky) or you can populate an f string by passing it into
eval
(which sounds like a security nightmare waiting to happen).So ultimately the only other choice I could think of boiled down to "use a template language" and since we're using Django, we essentially get one out of the box.
However I know that this may feel like overkill, and others may not share my opinions and evaluations of the other solutions, or even come up with a solution that I didn't think of. So this part is definitely flexible. I just wanted to get something working and in front of you and then iterate from there if needed.