Improve docstrings language in Metadata API #1815

ivanayov · 2022-02-01T22:45:10Z

This change updates some obvious and unnecessary fields docs in the
Metadata API with more despriptive details, as well as unifies
syntax between different classes and methods

Addresses #1600

The code follows the Code Style Guidelines
[n/a] Tests have been added for the bug fix or new feature
[n/a] Docs have been added for the bug fix or new feature

ivanayov · 2022-02-01T22:50:27Z

tuf/api/metadata.py

+            fields. It guarantees that input containing unknown fields will
+            not break serialization from and deserialisation into a Metadta
+            object and thus aligns with the TUF specification implication that
+            unrecognized attribute-value fields should be supported
        snapshot_meta: Meta information for snapshot metadata.


This is one I wondered whether to edit or not. It relates to obvious and unnecessary information, but on the other side it's simple and correct. I wondered what's your opinion about explaining why it's available in the API - do you find such clarification good or rather unnecessary?

(reply to another comment)
Yeah documenting meta in a comprehensive manner requires a lot of specification context so maybe we leave it as is... The MetaFile type does clearly document its content after all.

coveralls · 2022-02-01T22:50:37Z

Pull Request Test Coverage Report for Build 1811764274

0 of 0 changed or added relevant lines in 0 files are covered.
No unchanged relevant lines lost coverage.
Overall coverage remained the same at 98.343%

Totals
Change from base Build 1811698342:	0.0%
Covered Lines:	1118
Relevant Lines:	1134

💛 - Coveralls

jku

I wish this was in multiple commits so we could easily decide to only take some changes now and leave others for discussion... This issue should maybe have been split by change type to make that clearer that things like consistent double-backticks are easier to find a solution to than other issues (like consistent use of article where we need to decide what we want). The changes do conflict with each other so I can see why you did them all at once.

Anyway, I'll review but let's discuss if it makes sense to split this or to continue tweaking this PR.

There's a lot of good changes here, just commenting on potential issues.

I'm not sold on the use of indefinite article everywhere. It's fine in some places but looks quite bad in others:
threshold: A number of keys required to...
length: A length of the target file in bytes
those do not look right

Also the use of indefinite article is not consistent: there are still places that use definite article instead and the commit even adds a few new definite articles (see TargetFile methods for examples). I don't think 100% consistency is required but I don't understand how the article is chosen...

Overall, I think the indefinite article does not work in many places: In my opinion we should not standardize on it. I would maybe suggest a default choice of no article (since it makes docstrings shorter) and diverging from that if the context requires it.

jku · 2022-02-02T07:31:55Z

tuf/api/metadata.py

@@ -118,14 +118,14 @@ def __init__(self, signed: T, signatures: Dict[str, Signature]):

    @classmethod
    def from_dict(cls, metadata: Dict[str, Any]) -> "Metadata[T]":
-        """Creates Metadata object from its dict representation.
+        """Creates ``Metadata`` object from its dict representation.


We say "dict representation" a lot as if it's obvious what that means :)

I don't have a great alternative suggestion, just commenting.

Would it be better non-abbreviated, i.e. "dictionary representation"? I think I used dict to keep it on one line.
Or would "json/dict representation" help people better associate it with the format described in the spec?

I think "json/dict representation" is good. "dict" vs. "dictionary" is quite similar, while "json representation" should be obvious for everyone.

tuf/api/metadata.py

jku · 2022-02-02T08:29:21Z

tuf/api/metadata.py

+            fields. It guarantees that input containing unknown fields will
+            not break serialization from and deserialisation into a Metadta
+            object and thus aligns with the TUF specification implication that
+            unrecognized attribute-value fields should be supported
        snapshot_meta: Meta information for snapshot metadata.


(reply to another comment)
Yeah documenting meta in a comprehensive manner requires a lot of specification context so maybe we leave it as is... The MetaFile type does clearly document its content after all.

tuf/api/metadata.py

ivanayov · 2022-02-02T18:12:12Z

@jku Thank you for the review! I've split to several commits, as you advice. It's a bit harder to address feedback, as it should go to a specific commit in a single-file update, but I agree it's a better history at the end.

tuf/api/metadata.py

ivanayov · 2022-02-03T15:13:05Z

Python setup failed on windows.

jku

thanks for the nice commits: I was expecting to suggest moving some commits to a later PR... but I think I actually agree with all of these, they seem to be clear improvements. Only thing that is a required fix is a rebase (merge is not currently clean)

There are things to improve in the docs but this is a big PR already and there's no need to fix everything at once, just leaving some comments:

I noticed some docstring errors and left comments: none of them seem to be your fault so feel free to either fix here or leave to next PR
The unrecognised_fields docstring feels about the right amount of detail to me -- maybe we 'd like to explain general concept in one place somewhere but individual docstrings look ok
Many arguments still explain "what" the argument is, not "why" it is: as an example we document signer argument with securesystemslib.signer.Signer implementation which does not explain why signer is an argument (what is it used for).

... but all of these can be handled in future improvements. I've re-run the tests after setup-python fixed itself: they look good.

tuf/api/metadata.py

lukpueh · 2022-02-07T12:10:35Z

Thanks for working on this, @ivanayov and for reviewing, @jku! Let me know if you want more eyes.

tuf/api/metadata.py

ivanayov · 2022-02-07T18:05:42Z

Thank you @jku ! I addressed your comments here, as they are related to the changes anyway.
Changed

signer: ``securesystemslib.signer.Signer`` implementation

to

signer: ``securesystemslib.signer.Signer`` implementation containing a
signature and a keyid of the key that signature was generated with.

The other changes are just the fixes that you proposed + moving some of the existing changes to the right commit (I was a bit lazy to do that before addressing the comments 😁).

There are still some

data: Metadata content.
...
filename: Path to write the file to.
etc.

but I think there's enough context around them and adding explanations could make the docs too verbose and might rather confuse the user by creating expectations of some deeper meaning behind. Do you think that those should be better explained? @lukpueh do you have some preference, too?

jku

Thank you. I think I would like to leave the signer argument improvements for later: It is significantly more difficult to explain "why" than "what" -- and this PR is long enough.

In practice:

signer: ``securesystemslib.signer.Signer`` implementation containing a
signature and a keyid of the key that signature was generated with.

this is slightly incorrect (signer does not contain a signature, it produces signatures) and still doesn't explain why there is a signer argument. Could you just leave the original description in and then we can continue tweaking these later?

This change unifies quotes to double backtick across docs in the Metadata API in order to provide better visualisation Signed-off-by: Ivana Atanasova <[email protected]>

This change unifies wording across docs in the Metadata API, like Args vs. Arguments and same repetitive descriptions written differently in different classes/methods Signed-off-by: Ivana Atanasova <[email protected]>

This change unifies as mush as the context allows and improves the use of definite vs. indefinite vs. no article across docs in the Metadata API. It sticks to no article in most cases for simplisity and readability, but leaves definite article where it's strictly necessary Signed-off-by: Ivana Atanasova <[email protected]>

lukpueh · 2022-02-08T10:42:54Z

There are still some
data: Metadata content.
...
filename: Path to write the file to.
etc.
but I think there's enough context around them and adding explanations could make the docs too verbose and might rather confuse the user by creating expectations of some deeper meaning behind. Do you think that those should be better explained? @lukpueh do you have some preference, too?

I think both are okay. Especially the latter is actually adds useful information not provided by the arg name alone.

This change updates some obvious and unnecessary fields docs in the Metadata API with more despriptive details Signed-off-by: Ivana Atanasova <[email protected]>

ivanayov · 2022-02-08T10:44:02Z

@jku I removed the change.

jku · 2022-02-08T10:56:29Z

Thanks!

ivanayov commented Feb 1, 2022

View reviewed changes

jku reviewed Feb 2, 2022

View reviewed changes

ivanayov force-pushed the metadata_docstrings_imprv branch from bda6099 to 9cde29a Compare February 2, 2022 17:21

ivanayov commented Feb 2, 2022

View reviewed changes

tuf/api/metadata.py Outdated Show resolved Hide resolved

ivanayov force-pushed the metadata_docstrings_imprv branch from 9cde29a to 399a6a3 Compare February 2, 2022 19:05

MVrachev reviewed Feb 3, 2022

View reviewed changes

tuf/api/metadata.py Outdated Show resolved Hide resolved

ivanayov force-pushed the metadata_docstrings_imprv branch from 399a6a3 to a6a13f5 Compare February 3, 2022 14:37

jku self-requested a review February 7, 2022 10:22

jku requested changes Feb 7, 2022

View reviewed changes

tuf/api/metadata.py Outdated Show resolved Hide resolved

tuf/api/metadata.py Outdated Show resolved Hide resolved

tuf/api/metadata.py Outdated Show resolved Hide resolved

jku reviewed Feb 7, 2022

View reviewed changes

tuf/api/metadata.py Outdated Show resolved Hide resolved

ivanayov force-pushed the metadata_docstrings_imprv branch 2 times, most recently from a2893bd to 8b54971 Compare February 7, 2022 17:44

jku requested changes Feb 8, 2022

View reviewed changes

ivanayov force-pushed the metadata_docstrings_imprv branch from 8b54971 to 4a6ed8c Compare February 8, 2022 10:33

ivanayov added 3 commits February 8, 2022 12:37

Improve docstrings language in Metadata API - quotes

0feec0c

This change unifies quotes to double backtick across docs in the Metadata API in order to provide better visualisation Signed-off-by: Ivana Atanasova <[email protected]>

Improve docstrings language in Metadata API - wording

40aef7e

This change unifies wording across docs in the Metadata API, like Args vs. Arguments and same repetitive descriptions written differently in different classes/methods Signed-off-by: Ivana Atanasova <[email protected]>

Update repetitive docstrings language in Metadata API

9169fcb

This change updates some obvious and unnecessary fields docs in the Metadata API with more despriptive details Signed-off-by: Ivana Atanasova <[email protected]>

ivanayov force-pushed the metadata_docstrings_imprv branch from 4a6ed8c to 9169fcb Compare February 8, 2022 10:43

ivanayov mentioned this pull request Feb 8, 2022

Metadata API: Improve docstrings language #1600

Closed

jku approved these changes Feb 8, 2022

View reviewed changes

lukpueh mentioned this pull request Feb 8, 2022

verify_delegate() document ValueError and TypeError #1838

Merged

3 tasks

jku merged commit b497180 into theupdateframework:develop Feb 8, 2022

MVrachev mentioned this pull request Feb 8, 2022

Black doesn't warn about long lines inside function docstrings #1841

Open

This was referenced Mar 16, 2022

Improve docstrings in Metadata API to be more descriptive #1843

Merged

docs: standardize how we refer to methods in docstrings #1595

Closed

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

Improve docstrings language in Metadata API #1815

Improve docstrings language in Metadata API #1815

ivanayov commented Feb 1, 2022 •

edited

Loading

ivanayov Feb 1, 2022 •

edited

Loading

jku Feb 2, 2022 •

edited

Loading

coveralls commented Feb 1, 2022 •

edited

Loading

jku left a comment •

edited

Loading

jku Feb 2, 2022

lukpueh Feb 8, 2022

ivanayov Feb 8, 2022

jku Feb 2, 2022 •

edited

Loading

ivanayov commented Feb 2, 2022 •

edited

Loading

ivanayov commented Feb 3, 2022

jku left a comment •

edited

Loading

lukpueh commented Feb 7, 2022

ivanayov commented Feb 7, 2022 •

edited

Loading

jku left a comment •

edited

Loading

lukpueh commented Feb 8, 2022

ivanayov commented Feb 8, 2022

jku commented Feb 8, 2022

Improve docstrings language in Metadata API #1815

Improve docstrings language in Metadata API #1815

Conversation

ivanayov commented Feb 1, 2022 • edited Loading

ivanayov Feb 1, 2022 • edited Loading

Choose a reason for hiding this comment

jku Feb 2, 2022 • edited Loading

Choose a reason for hiding this comment

coveralls commented Feb 1, 2022 • edited Loading

Pull Request Test Coverage Report for Build 1811764274

💛 - Coveralls

jku left a comment • edited Loading

Choose a reason for hiding this comment

jku Feb 2, 2022

Choose a reason for hiding this comment

lukpueh Feb 8, 2022

Choose a reason for hiding this comment

ivanayov Feb 8, 2022

Choose a reason for hiding this comment

jku Feb 2, 2022 • edited Loading

Choose a reason for hiding this comment

ivanayov commented Feb 2, 2022 • edited Loading

ivanayov commented Feb 3, 2022

jku left a comment • edited Loading

Choose a reason for hiding this comment

lukpueh commented Feb 7, 2022

ivanayov commented Feb 7, 2022 • edited Loading

jku left a comment • edited Loading

Choose a reason for hiding this comment

lukpueh commented Feb 8, 2022

ivanayov commented Feb 8, 2022

jku commented Feb 8, 2022

ivanayov commented Feb 1, 2022 •

edited

Loading

ivanayov Feb 1, 2022 •

edited

Loading

jku Feb 2, 2022 •

edited

Loading

coveralls commented Feb 1, 2022 •

edited

Loading

jku left a comment •

edited

Loading

jku Feb 2, 2022 •

edited

Loading

ivanayov commented Feb 2, 2022 •

edited

Loading

jku left a comment •

edited

Loading

ivanayov commented Feb 7, 2022 •

edited

Loading

jku left a comment •

edited

Loading