sphinx-needs icon indicating copy to clipboard operation
sphinx-needs copied to clipboard

use a class for 'need info' instead of a dictionary

Open danieleades opened this issue 2 years ago • 5 comments

the 'need info' should be a class rather than a dictionary

  • attributes are strongly-typed
  • methods can be added to the class for encapsulation

for example, "trimmed_title" is an attribute in the need info dict which is added during creation in the api.add_need method. This could instead be a method on a NeedInfo class, which would improve encapsulation.

I've created a proof of concept of this approach using dataclasses based on the 'todo extension' example, but you could also use plain classes - https://github.com/danieleades/sphinx-todo/blob/1d17716a827b8c7cbcb12b318a471658487b5d7d/src/sphinx_todo/init.py#L50

danieleades avatar Jun 07 '22 09:06 danieleades

If this was of interest I would need some help to pull it together. The 'needs info' is created in a couple of different places in the codebase, and different attributes are added at different times (it's pretty complex right now). I could perhaps make a start on this, but would need some support.

This doesn't need to be a breaking change, as the construction of needs could be entirely internal to the library, however it would ultimately be tidier to expose the class as part of the external api, and have 'add_need' accept the class, rather than the mass of args that it currently takes.

thoughts?

danieleades avatar Jun 07 '22 09:06 danieleades

Puhh, that would be a lot of work to do. From the technical point of view, this makes totally sense. But I'm afraid that there are so many pieces in the code, which are working with this object, that really every directive/feature will be affected. And also the review will be huge, as this review can't be done step by step.

Not sure if I can help here much, as my time is limited.

I would also wait 1-2 months with such a refactoring, as we are just preparing release 1.0, which will introduce some new features and therefore will have some/many follow-up bug fixes and code changes.

To be honest, I'm not sure if such a change is worth the effort.

danwos avatar Jun 07 '22 09:06 danwos

I have a couple of long-haul flights coming up, so I might have some spare time on my hands ;)

I'll see how I go.

Converting dictionary access calls to class attribute calls will be largely mechanical. However the code changes around 1.0 are very likely to lead to churn and difficulty rebasing, i agree with you there.

danieleades avatar Jun 07 '22 10:06 danieleades

Please go like this. With #611 I would have the demand, to have an API to automatically translate a need in a pure dict(str, str). This would easly possible with a class and a methode for.

How can I help?

PhilipPartsch avatar Jun 13 '22 21:06 PhilipPartsch

Please go like this. With #611 I would have the demand, to have an API to automatically translate a need in a pure dict(str, str). This would easly possible with a class and a methode for.

How can I help?

i think an interim solution is to use a TypedDict to provide typings without any API changes. Once that's done, migrating from TypedDict to a class or dataclass is relatively trivial

danieleades avatar Jun 14 '22 00:06 danieleades