Page MenuHome

Latest changes to the Blender Conference website
AcceptedPublic

Authored by Sem Mulder (SemMulder) on Wed, Sep 25, 4:33 PM.

Diff Detail

Event Timeline

With all the type annotations (which I'm a big fan of) it's important to add mypy to the tests. Probably enough to copy & adjust https://developer.blender.org/source/blender-dev-fund/browse/master/blender_fund/tests.py for that.

conference_main/context_processors.py
12

settings is also used for Django's django.conf.settings in some of our other projects (for example dev fund). To avoid confusion, maybe site_settings would be a more suitable key.

conference_main/models.py
40

👍

111

In the unit tests cancelled is treated as 'accepted but later cancelled, but still accepted in a way'. I think this isn't immediately obvious from just the word 'Cancelled', so maybe it's worth a comment.

conference_main/permissions.py
16

I agree with this reasoning, and happy to see this here.

29

This is a bit tricky, as is_staff only indicates that the user is allowed to use the admin, and doesn't imply any other permissions. In this file, however, it is used as a permission, in which case is_superuser is probably the one you want to pick instead, or defer to the Django permission system to check for a more specific permission for event moderators.

43

AFAIK we have a line length limit of 100 for most projects. We probably should add a setup.cfg that configures some tools, like this one in Flamenco.

conference_main/templates/conference_main/base.pug
53

I also prefer en-GB, but on Blender sites we use en-US, so 'Favorites'.

conference_main/templates/conference_main/festival/entries/list.pug
14

Why the # and not just #?

conference_main/templates/conference_main/presentations/detail.pug
10

Don't use <a href='#'>, use <button> instead. Links should always have a valid URL.

conference_main/tests/test_event.py
77

Test with some non-ASCII characters in the name at least once somewhere.

conference_main/views/festival_entry.py
92

Same note about User.is_staff as in the permissions module.

142

Why the randomisation?

Sybren A. Stüvel (sybren) requested changes to this revision.Wed, Sep 25, 5:54 PM
This revision now requires changes to proceed.Wed, Sep 25, 5:54 PM
This revision was not accepted when it landed; it landed in state Needs Revision.Thu, Sep 26, 9:04 AM
This revision was automatically updated to reflect the committed changes.
Sem Mulder (SemMulder) marked 9 inline comments as done.Thu, Sep 26, 10:31 AM

I addressed the small things. There are now two things left to do:

  • make the favorite star use a checkbox,
  • use Django permissions for the admin/moderator/staff permissions.
conference_main/models.py
111

Added a comment.

conference_main/permissions.py
29

I think using the Django permission system here instead of the user.is_staff might be nice. That way we use Django's permission system for managing permissions for admins/staff/moderators and we use this file for managing permissions for users. The only thing is then that we have two systems called the "permission" system, but I can just explain that in the docstring here too.

43

Added the max-line-length to setup.cfg and reformatted the violating lines.

How do you feel about introducing: https://github.com/psf/black? I personally really like it because it removes any discussion about formatting and I haven't seen any issues with readability because of it.

conference_main/templates/conference_main/festival/entries/list.pug
14

This is needed because PyPugJS messes up the escaping.
Quoting https://pugjs.org/language/interpolation.html:

If you need to include a verbatim #{, you can either escape it, or use interpolation. (So meta!)

But doing so raises an exception within PyPugJS.

conference_main/views/festival_entry.py
142

The idea is that it makes voting slightly more unbiased, will add a comment.

Sem Mulder (SemMulder) marked 5 inline comments as done.Thu, Sep 26, 11:52 AM

Added a reusable favorite star component. Only the permissions left now.

conference_main/templates/conference_main/presentations/detail.pug
10

I added a reusable favorite star component.

conference_main/permissions.py
43

Black looks good to me. Blender itself also recently moved to automatic code formatting, and I really like it (I'm also used to it in Go, where auto-formatting has been the norm for years).

conference_main/templates/conference_main/festival/entries/list.pug
14

Ah yes, it's because of the { after it. Makes sense once you know ;-)

Sem Mulder (SemMulder) marked 5 inline comments as done.Thu, Sep 26, 12:22 PM

We are also now handling the permissions quite nicely. I'll add black and mypy as part of the tests now.

conference_main/permissions.py
43

Nice, I'll add some configs to the repo and a check that everything is propely formatted as part of the tests.

Sem Mulder (SemMulder) marked an inline comment as done.

Reopening because it was closed accidentally.

This should fix all raised issues.

I notice that you changed all the single quotes to double quotes. Is this the Black default? We generally mimic Python's own behaviour in our sources, so single quotes for strings unless the string contains a single quote, then use double quotes:

Python 3.7.3 (default, Aug 20 2019, 17:04:43) 
[GCC 8.3.0] on linux
Type "help", "copyright", "credits" or "license" for more information.
>>> "hey"
'hey'
>>> "he said 'hey'"
"he said 'hey'"
conference_main/templates/conference_main/presentations/detail.pug
10

Now it's a <div> with an onclick handler. In other words, the browser doesn't know it is an interaction element, it can't be used with the keyboard, etc. What's wrong with a <button> element? I'm really not a fan of the everything-is-a-div-let's-ignore-semantics approach.

Now using a <button> for the favorite star and skipping string
normalization in Black.

Sem Mulder (SemMulder) marked an inline comment as done.Fri, Sep 27, 9:32 AM

I notice that you changed all the single quotes to double quotes. Is this the Black default? We generally mimic Python's own behaviour in our sources, so single quotes for strings unless the string contains a single quote, then use double quotes:

Yeah, it's the default. See https://black.readthedocs.io/en/stable/the_black_code_style.html#strings for their reasoning. I set a flag that it leaves the quotes alone now.

conference_main/templates/conference_main/presentations/detail.pug
10

This one scared me: https://stackoverflow.com/questions/469059/button-vs-input-type-button-which-to-use. But that might be out of date. Made it a <button> now anyway.

Sem Mulder (SemMulder) marked an inline comment as done.Fri, Sep 27, 9:35 AM
This revision was not accepted when it landed; it landed in state Needs Review.Fri, Sep 27, 9:36 AM
This revision was automatically updated to reflect the committed changes.

Phabricator is still closing this differential whenever I push, @Sybren A. Stüvel (sybren) any idea why?

This revision was not accepted when it landed; it landed in state Needs Review.Fri, Sep 27, 2:11 PM
This revision was automatically updated to reflect the committed changes.
Sybren A. Stüvel (sybren) accepted this revision.
This revision is now accepted and ready to land.Fri, Sep 27, 3:59 PM

By default phabricator autocloses for all branches. I've configured it to only do it for the master branch of BCONF now.