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

Better support for status and initial support for partstat #1271

Merged
merged 5 commits into from
Nov 15, 2023

Conversation

geier
Copy link
Member

@geier geier commented Jun 15, 2023

No description provided.

@madduck
Copy link
Contributor

madduck commented Jun 22, 2023

I can confirm that this works as intended. Will take discussion back to #580 though.

@property
def _status_str(self) -> str:
if self.status == 'CANCELLED':
statusstr = ' ' + self.symbol_strings['cancelled']
Copy link
Contributor

Choose a reason for hiding this comment

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

I'd like to make a case against adding a leading space, or at least to make this configurable. Such a leading space should really be part of the format specification, though I realise it's currently not possible to work with conditionals there.

However, {status: <1} will probably suit anyone better when the status symbol is to be used mid-field, whereas adding all the statuses to the end of the line won't make a difference anyway.

Copy link
Member Author

Choose a reason for hiding this comment

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

fixed

@geier
Copy link
Member Author

geier commented Oct 31, 2023

config file now takes several addresses per account, but we expect that only one will match.

@geier
Copy link
Member Author

geier commented Oct 31, 2023

@WhyNotHugo sorry for bombing you with review requests, feel free to ignore them.

@geier geier marked this pull request as ready for review October 31, 2023 00:53
@geier
Copy link
Member Author

geier commented Oct 31, 2023

This PR doesn't cover yet formatting events by their status as it's a bit more difficult to get this looking the same in khal and ikhal. I'd prefer to merge this without that now, because rebasing against master is becoming tiresome (not especially this branch, but keeping several non-merged features branches around is getting tiring).

Copy link
Member

@WhyNotHugo WhyNotHugo left a comment

Choose a reason for hiding this comment

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

If I understand correctly, the CALDAV:calendar-user-address-set property allows querying the address of the owner of a calendar.

This should enable fetching the email address of the calendar owner via vdirsyncer.

Perhaps treating this as calendar metadata might be more future compatible?

@geier
Copy link
Member Author

geier commented Oct 31, 2023

@WhyNotHugo interesting. We should definitively support that, but probably only as a default. At least one of my caldav servers has an email address set that nobody sends email to.

@geier geier enabled auto-merge November 15, 2023 20:08
@geier geier merged commit 214886d into master Nov 15, 2023
6 checks passed
@geier geier deleted the feature/status branch November 15, 2023 20:08
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.

3 participants