Skip to content
This repository has been archived by the owner on Jun 25, 2024. It is now read-only.

Update systemd module to include user units #374

Merged

Conversation

KarolosLykos
Copy link
Contributor

Issue: #294

Added an optional busType parameter in the Service function on the systemd module in order to not introduce a breaking change.

I tested locally and seems to work just fine although, I would really appreciate any ideas on how to write tests for this.

@soumya92
Copy link
Owner

soumya92 commented Apr 6, 2023

Since there are only two bus types, I think it would be cleaner to add UserService and UserTimer methods at top-level. WDYT?

@KarolosLykos
Copy link
Contributor Author

Since there are only two bus types, I think it would be cleaner to add UserService and UserTimer methods at top-level. WDYT?

I don't know..., adding UserService and UserTimer methods at the top-level it may result in unnecessary duplication. Additionally, there is another bus used in the tests :D, which would require further modifications.

@soumya92
Copy link
Owner

soumya92 commented Apr 6, 2023

The method bodies can just forward to a private method, that one you can freely change the api for. I just don't like the idea of using varargs to workaround the lack of overloads/optional arguments in go, especially in the public api.

It can lead to calls like Timer('foo', System, User), which would compile but only use the system bus. We could work around that by panic if len(busTypes) > 1, but might be easier to just avoid that by using two more top-level functions in the API.

@KarolosLykos
Copy link
Contributor Author

The method bodies can just forward to a private method, that one you can freely change the api for. I just don't like the idea of using varargs to workaround the lack of overloads/optional arguments in go, especially in the public api.

It can lead to calls like "Timer('foo', System, User), which would compile but only use the system bus. We could work around that by panic` if len(busTypes) > 1, but might be easier to just avoid that by using two more top-level functions in the API.

Hi @soumya92 , updated in the last commit. Waiting for your feedback. Cheers

@soumya92 soumya92 merged commit 1feca84 into soumya92:main Apr 6, 2023
Sign up for free to subscribe to this conversation on GitHub. Already have an account? Sign in.
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

2 participants