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

Services reference common configuration options #1080

Open
wants to merge 1 commit into
base: develop
Choose a base branch
from

Conversation

ryneeverett
Copy link
Collaborator

  • Add a test that all services reference the common configuration options (at least vicariously). The exception is add_tags, which is done during the issue aggregation phase.
  • Add an UnsupportedOption type to the configuration schema which throws a validation error if the given value is changed from it's default.
  • Set the unimplemented options to this UnsupportedOption value.

One of the effects of no longer requiring the get_owner method in #1070 which I wasn't pleased with is that there is no longer an impetus for the service developer to implement these common configuration options. However, I also learned in that PR that making imperfect but nonetheless useful tests by regexing the code can cheaply make some loose guarantees that software follows some basic guidelines.

So this should restore the virtue that unimplemented features need to be noticed and suggests an even more explicit statement that they are unsupported. Furthermore, this gives users a validation error when they configure one of these unsupported features, rather than failing silently as the #1070 commit notes was always the case:

One might suspect that eliminating these NotImplementedError's would eliminate the warnings from services that do not support only_if_assigned, but in fact these would only get raised if the service itself called self.include, which they do not. So a user trying to use these options already would find them silently ignored, which isn't great but is a problem for another day.

Today is that day!

- Add a test that all services reference the common configuration
  options (at least vicariously). The exception is add_tags, which is done
  during the issue aggregation phase.
- Add an UnsupportedOption type to the configuration schema which throws
  a validation error if the given value is changed from it's default.
- Set the unimplemented options to this UnsupportedOption value.

One of the effects of no longer requiring the get_owner method in GothenburgBitFactory#1070
which I wasn't pleased with is that there is no longer an impetus for
the service developer to implement these common configuration options.
However, I also learned in that PR that making imperfect but nonetheless
useful tests by regexing the code can cheaply make some loose guarantees
that software follows some basic guidelines.

So this should restore the virtue that unimplemented features need to be
noticed and suggests an even more explicit statement that they are
unsupported. Furthermore, this gives users a validation error when they
configure one of these unsupported features, rather than failing
silently as the GothenburgBitFactory#1070 commit notes was always the case:

> One might suspect that eliminating these NotImplementedError's would eliminate the warnings from services that do not support only_if_assigned, but in fact these would only get raised if the service itself called self.include, which they do not. So a user trying to use these options already would find them silently ignored, which isn't great but is a problem for another day.

Today is that day!
@ryneeverett
Copy link
Collaborator Author

@scross01 Note that this will change the default priority of logseq tasks from None to the default_priority configuration value, which defaults to 'M'. (I doubt that's an optimal behavior but that's what all the other services have always done.) If you don't like this change you should be able to set the default_priority to None, but per #426 that may be broken at the moment.

@scross01
Copy link
Contributor

scross01 commented Dec 31, 2024

@ryneeverett setting logseq.default_priority = in the service config using the default get_priority() method is working (on develop branch).

Is there reason why bugwarrior sets the default for default_priority to 'M'? I would have thought that '' or None would be better default. Using taskwarrior CLI there's no default priority for new tasks.

Also, while testing I noticed that changes to logseq.default_priority does not update existing tasks, when doing a new pull only new tasks get the updated default priority. Looking at the DB update code this is because priority is set as a default static field. Setting static_fields = in the general config resolves this.

@ryneeverett
Copy link
Collaborator Author

Is there reason why bugwarrior sets the default for default_priority to 'M'? I would have thought that '' or None would be better default.

I agree and I think most of the community would agree. See #780 though. Note that I did not close that issue but I do think we would need to be thoughtful about how we made such a change. Feel free to open an issue about changing that default.

Also, while testing I noticed that changes to logseq.default_priority does not update existing tasks, when doing a new pull only new tasks get the updated default priority. Looking at the DB update code this is because priority is set as a default static field. Setting static_fields = in the general config resolves this.

I do think having default_priority in static_fields is a reasonable default because it allows you to manually change the priority of bugwarrior-managed tasks without them being reset on your next pull. That property of default_priority should be documented though because you shouldn't have to read the code to find out why it doesn't update existing tasks.

ryneeverett added a commit to ryneeverett/bugwarrior that referenced this pull request Jan 1, 2025
In GothenburgBitFactory#1080 it was pointed out that it isn't obvious why changes to
default_priority aren't applied to existing tasks, so let's explain that
at the point where it is most useful to know.
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