-
-
Notifications
You must be signed in to change notification settings - Fork 1.7k
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
programs.khal: remove extra new line on config #5221
Conversation
thanks a lot for cleaning up my mess <3 |
I must confess I am a bit surprised to see a contribution just to cleanup newlines
I could say that I was already "warm", working on a small patch to khard
module. Which got me excited to learn how to contribute to a project I
appreciate.
you could add "addresses" support
Thanks for the pointer, I thought it was a new module here on home-manager :D.
Checking the khal documentation, I think it is not related to "birthday"
calendars [^1], since they wouldn't have event invites, I guess... Or the
approach here should be to not impose such deductions but let anything possible
on upstream to be possible through these modules.
Anyhow I will add the feature in a bit, I already sketched something.
In case it isn't obvious (/s), I'm quite new to this game, any feedback &
patience shown is appreciated.
[^1]: thus should be implemented for `accounts.calendar.accounts.<>.khal.` not
the `accounts.contact.accounts.<>.khal`
|
I've just added another commit for the aforementioned feature, once approved I
can squeeze them to a single commit if it is required.
|
LGTM but I'll let @teto have the last word 🙂 |
Just removed the emtpy line you suggested, thanks for the review.
When @teto gives his opinion I would squash all to a single commit.
|
@octvs you are the goat ! Looks fine apart from the formatting that is off as the CI tells you. Format them and we are all good. You dont have to squash, we can do it from the github UI, and actually the home-manager project forces the squash so we dont even have a choice. |
- Add `accounts.<calendar|contact>.accounts.<name>.khal.addresses` option to enable new configuration option, "addresses", from khal, which is used for showing participation status [1]. - Remove mistaken new line in khal implementation, refer to [2]. - Make additions to the existing test case to check the new addresses feature. And remove the empty lines in expected configs. [1]: https://khal.readthedocs.io/en/latest/configure.html#the-calendars-section [2]: nix-community#5192 (review)
Thank you for the kind compliment :).
formatting that is off as the CI tells you.
Oh I've missed that completely, that is to due to my globally set formatter
getting the upper hand via format-on-save, now should be fixed.
home-manager project forces the squash so we dont even have a choice.
I can't say I'm 100% clear on this, I would probably grasp the workflow more in
time. For now sticking with habits, I squash to clear the commit msgs,
meanwhile also rebased to the current HEAD. Sorry for triggering another CI run
here, especially if it is bad practice.
Thanks to both of you, for the review & your time.
|
thank you for agreeing to my unsreasonable requests and welcome to home-manager ;) |
Description
Refer to the commit msg.
Checklist
Change is backwards compatible.
Code formatted with
./format
.Code tested through
nix-shell --pure tests -A run.all
ornix develop --ignore-environment .#all
using Flakes.Test cases updated/added. See example.
Commit messages are formatted like
See CONTRIBUTING for more information and recent commit messages for examples.
If this PR adds a new moduleAdded myself as module maintainer. See example.Maintainer CC
@teto