useblocks / sphinx-needs

Adds needs/requirements to sphinx
https://sphinx-needs.readthedocs.io/en/latest/index.html
MIT License
210 stars 66 forks source link

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

Open danieleades opened 2 years ago

danieleades commented 2 years ago

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

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 commented 2 years ago

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?

danwos commented 2 years ago

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.

danieleades commented 2 years ago

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.

PhilipPartsch commented 2 years ago

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?

danieleades commented 2 years ago

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