[Lotram]

This PR is a suggestion of data models refactoring.

Why ?

1. Using our own Task model decreases the dependency to taskw, and improves typing / readability, instead of using plain dicts. Having our own abstraction simplifies the changes needed to switch the dependency from taskw to taskchampion.
2. The Issue (and child classes) becomes less useful, it does not hold any specific logic, I suggest an implementation where it's merged into Service.
3. This gives us a clearer split between data coming from external services (Service, record) and data inserted in taskwarrior

What's inside this PR

1. The Task and Udas models
2. Use these models inside Service / Issue, so we use that instead of plain dicts (in db.py, collect.py etc)
3. Merge Issue inside Service, as there is no clear difference between the two
4. Move several methods (the ones where self is only used to access the config) out of Service

Commits 3 and 4 are optional, I think they're improvements, but not necessary.
I only modified Linear to demonstrate this.
The idea is not to review it in details for now, there are probably some bugs, but to discuss the overall shape of the changes.

What's next ?

If we proceed with this changes, I would then use Task even more in db.py, and try avoid using dicts.
We could also replace calls to taskw.task.FIELDS by our own Task object

[ryneeverett]

1. Using our own Task model decreases the dependency to taskw, and improves typing / readability, instead of using plain dicts. Having our own abstraction simplifies the changes needed to switch the dependency from  taskw to taskchampion.
...

If we proceed with this changes, I would then use Task even more in db.py, and try avoid using dicts. We could also replace calls to taskw.task.FIELDS by our own Task object

This makes sense to me in principle, but it doesn't seem like the decreased dependency on taskw has manifested in this PR. I'm kind of thinking a well-scoped PR might be focused on completely abstracting away taskw usage to prepare for #1078 . What do you think?

2. The `Issue` (and child classes) becomes less useful, it does not hold any specific logic, I suggest an implementation where it's merged into Service.

I think there is a bit of a principled distinction between the two (described in the original doc-comments) but it might not be useful and probably isn't consistently enforced. I also think it's going to be hard to evaluate whether this change is an improvement if it is done at the same time as the Task model. Maybe we can hold off on this in order to reduce the scope of the Task model transition and so that we can better focus on getting it right if/when we do it?

[ryneeverett]

Thinking about this a bit more, I'd note that I think we are essentially on the same page. I see where removing UDA's from Issue makes that class even less useful and suggests elimination. I'd still suggest pushing that out to a separate PR though.

[Lotram]

This makes sense to me in principle, but it doesn't seem like the decreased dependency on taskw has manifested in this PR. I'm kind of thinking a well-scoped PR might be focused on completely abstracting away taskw usage to prepare for #1078 . What do you think?

Yes, as I see it, removing taskw dependency would be enabled by introducing our own Task model, thus in a follow-up PR. The main point of this current PR is my point 3: clearer split between external data and taskwarrior data.

But I definitely can remove the "Issue into Service" changes from this PR, and we'll do that later.
In my opinion, the main reason to do everything now is to avoid doing multiple breaking changes: I'd rather have 1 really big API change than multiple smaller ones. But as long as there is no released versions in between, it's not a problem, and we can have multiple PRs, I'm ok with that

[ryneeverett]

3. This gives us a clearer split between data coming from external services (Service, record) and data inserted in taskwarrior.

Could you elaborate on this a little bit? I noticed that some of the naming was more consistent which i appreciated, but I'm assuming you mean something more abstract/conceptual...

[Lotram]

Could you elaborate on this a little bit? I noticed that some of the naming was more consistent which i appreciated, but I'm assuming you mean something more abstract/conceptual...

• The validation is done bit by bit in several places: some in get_issues, some in to_taskwarrior, and some later in collect.py (aggregate_issues, TaskConstructor`, etc).
• Said differently, or at least on the same subject, there is currently no single source of truth to handle parsing / validating the data before inserting it to taskwarrior (and especially the "generic" / non-udas data). Each Issue subclass is responsible for calling the right parsers (e.g parse_date), thus a lot of code duplication + potential errors
• Because of this, there is absolutely no guarantee on what's inside the dict returned by to_taskwarrior and used in collect.py / db.py. That's why I had to remove some other validation logic from db.py in an earlier MR, there was nothing telling us the data was impossible (I remember at least a check for priority = '' which was done in multiple places).

My point is to have a single "entrypoint" (not sure it's the right word here), the Task Model, for all the collect.py / db.py logic. Each service should only be responsible for retrieving the data from their source, and comply with the Task API contract. Once we have this Task object, we should be able to assume all the data has been properly validated (datetimes have the right format, priority is the right enum, etc)

At some point, I think we'll also be able to add all the "update an existing task" logic inside that model (e.g. updating the tags, the annotations, etc)

[ryneeverett]

Gotcha. I do see the benefits of centralizing validation. I think transitioning to the Task model is an acceptable PR scope.

[Lotram]

@ryneeverett I finally finished working on this MR. As all the services used to have their own implementation (e.g. no shared taskwarrior field definition, to_taskwarrior returning a simple dict), with no proper common interface, it's not easy to ensure I did not break anything.

I kept the Issue class for now, we can remove it in another MR.

[ryneeverett]

Sorry I've been too busy to look at this, but please rebase.

[ryneeverett]

What would you think about dropping prefixes from the Udas field names and automatically adding them here? My thought is it easier to enforce consistency and makes the UDA's classes more readable (githubtitle -> title).

Suggested change

	targets_udas.update(get_service(service).TASK_SCHEMA.serialize())
	service = get_service(service)
	targets_udas.update({
	f"{service.CONFIG_SCHEMA.service if service.CONFIG_SCHEMA.service != "azuredevops" else "ado"}{key}": value
	for key, value in service.TASK_SCHEMA.serialize()})

[Lotram]

To be honest, I think the best solution would be to compute a unique bugwarrior_id to identify each task in taskwarrior, then get rid of the prefixes entirely (e.g. an url should be called url, no matter the underlying service).

But your suggestion is reasonable, and a good first step towards the prefix removal

[Lotram]

I used a UDA_PREFIX attribute though, as we don't always have access to the service, if I'm not mistaken.

[Lotram]

This is not perfect, maybe I'll find a way to re-use the Config.service attribute (though it would be fixed by dropping the prefix entirely, too)

[ryneeverett]

The uda's were prefixed long before the UNIQUE_KEY method of differentiating bugwarrior-tracked tasks was developed. I'm guessing the main motivation was to avoid namespace conflicts with core attributes. Jira was one of the first services and I see id and status would conflict with the core attributes. But maybe only the conflicting fields ought to be prefixed? At the very least we'd want to add a Udas validator that makes sure no fields conflict with core attributes. IMO this is best left for another PR.

we don't always have access to the service

Huh? Do you mean in another context or this line in db.py?