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

Room tenancies, find lost commits #708

Merged
merged 2 commits into from
Apr 5, 2024
Merged

Room tenancies, find lost commits #708

merged 2 commits into from
Apr 5, 2024

Conversation

hw0lff
Copy link
Contributor

@hw0lff hw0lff commented Apr 2, 2024

With this PR, the person_id is also shown in the webview, even if no user could be mapped.

Comment on lines 616 to 622

def tenancy_person_id(tenancy) -> int | None:
return tenancy.person_id if tenancy.person_id is not None else None

def tenancy_user_button(tenancy) -> BtnColResponse | None:
return user_button(tenancy.user) if tenancy.user is not None else None

Copy link
Collaborator

Choose a reason for hiding this comment

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

Using nested functions with one invocation is nonsense unless you have a really good reason. Here, you could've used a plain variable if you don't like the line length.

Otherwise, if we want to do null coalescence anyway, apart from fancy solutions we could just bake the null coalescence into user_button (changing its signature to User | None -> str | None)

Copy link
Contributor Author

Choose a reason for hiding this comment

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

I'm not sure a variable would be applicable here since it is used in a list comprehension. Avoiding long lines was the intention but let's inline these functions if there is not a better way to do it.

Copy link
Collaborator

Choose a reason for hiding this comment

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

Right, sorry, I overlooked the list comprehension. Then I'm not sure and I would just vouch for inlining, purely because weakening user_button changes semantics.

Copy link
Collaborator

Choose a reason for hiding this comment

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

I mean the other way would be the fancy bs way:

def lift[A, B](f: Callable[[A], B | None]) -> Callable[[A | None], B | None]:
    def _lifted(a: A | None) -> B | None:
        return f(a) if a is not None else None
    return _lifted

and then use lift(user_button)(tenancy.user)

Copy link
Collaborator

Choose a reason for hiding this comment

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

(beware, untested)

Comment on lines 619 to 621
inhabitant=user_button(tenancy.user) if tenancy.user else None,
inhabitant=user_button(tenancy.user) if tenancy.user is not None else None,
swdd_person_id=tenancy.person_id if tenancy.person_id is not None else None,
Copy link
Contributor Author

Choose a reason for hiding this comment

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

Can the condition if tenancy.user is not None else None be shortened to if tenancy.user else None or does it change the meaning?
Same question for tenancy.person_id in line 621.

Copy link
Collaborator

Choose a reason for hiding this comment

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

Yes, since sqlalchemy objects are falsy iff they are None.

@lukasjuhrich lukasjuhrich merged commit 3170a78 into develop Apr 5, 2024
7 checks passed
@FestplattenSchnitzel FestplattenSchnitzel removed their request for review April 5, 2024 16:41
@lukasjuhrich lukasjuhrich deleted the room-tenancies branch June 8, 2024 12:35
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