refactor sphinxcontrib.needs.api.need

[danieleades]

• simplify a few methods
• reduce duplication between directives/need and api/need
• format imports with isort
• format api/need using Black
• add type annotations

[danieleades]

I have so many questions after going through api/need. there's still a great deal of logic duplicated. For example, when a new need is created in directive/need the title is trimmed before passing to the add_need function, which also trims the title. Who's responsible for what? What is the relationship between the directive and the api code?

I think it would be much tidier if the needs_info dictionaries being passed around were a class instead of a plain old dict. Would be far more maintainable if the fields were typed members, instead of dictionary values. a lot of the manipulation done to the various fields would be a lot tidier as member functions. But that would be a breaking change to the API, so that's probably out of the question.

I'm deeply confused by a few blocks in api.need still. For starters, @danwos could you possible walk me through _merge_global_options? I feel like there's room to simplify that one

[danieleades]

    # Adding of basic Need node.
    ############################
    # Title and meta data information gets added alter during event handling via
    # process_need_nodes(). We just add a basic need node and render the rst-based
    # content, because this can not be done later.
    # style_classes = ['need', type_name, 'need-{}'.format(type_name.lower())]
    # # Used < 0.4.4
    style_classes = ["need", "need-{}".format(need_type_info.directive.lower())]
    if style:
        style_classes.append(style)

if this is only "Used < 0.4.4", can it be removed?

[danieleades]

rebased on master

[danieleades]

rebase on master

[danieleades]

that was a messy rebase. will see what the CI says

[jakehader]

Hi @danieleades, I looked through your changes and I personally like where this is going. Is there any momentum behind getting this PR reviewed and merged in?

[danieleades]

I wanted to do a little more, but it's probably worth merging as is.
I've not looked at this for a while, will give it a once over in the next couple of days

[danieleades]

rebased on master

[danieleades]

as far as i'm concerned this is mergeable. I'm not really intending to make any further changes on this pull request.

CI is failing on python3.5, seems like a dependencies issue, so i don't think it's anything to do with this pull request.

[danwos]

Reviewd it and it looks good.
I have solved the deps problem by another PR (which also contains some updates for the api.py file). #234

So I have introduced 2 conflictes, sorry.

[danieleades]

i'm tempted to abandon this, and maybe revisit another time. The rebase proved to be non-trivial

[danwos]

Not sure, the only change I made was to check if tags and links_string are really strings or already lists:

tag check

- tags = [tag.strip() for tag in re.split(";|,", tags)]
+ # tags should be a string, but it can also be already a list,which can be used.
+ if isinstance(tags, str):
+    tags = [tag.strip() for tag in re.split(";|,", tags)]

link check

-        for link in re.split(";|,", links_string):
+        # Check if links_string is really a string, otherwise it will be a list, which can be used
+        # without modifications
+        if isinstance(links_string, str):
+            link_list = re.split(";|,", links_string)
+        else:
+            link_list = links_string
+        for link in link_list:

You moved this part in an extra function, therefore it looks like a bigger change.
Would be happy to have this PR.

I can do it as well (at least the conflict is my fault), but I think I don't have permission to add commits to this PR / your fork, right?

[danwos]

Do we still need this?
It's quite old and not up to date.