Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

feat: Add docstring and code source information to Parameter #288

Merged
merged 8 commits into from
Jun 12, 2024

Conversation

has2k1
Copy link
Contributor

@has2k1 has2k1 commented Jun 10, 2024

This change gives the Parameter class the following attributes / propeties:

  • docstring
  • lineno
  • endlineno
  • parent
  • lines
  • linescollections
  • source

Currenlty these values are only populated for dataclasses, and they are None for regular classes and functions. This makes it possible to accesss this information for all dataclass parameters, especially those set as InitVar where it was not possible to get it since they are not members (attributes) of the class.

Issue #286: #286
Related to #252: #252

This change gives the Parameter class the following attributes /
propeties:

   - docstring
   - lineno
   - endlineno
   - parent
   - lines
   - linescollections
   - source

Currenlty these values are only populated for dataclasses, and they are
None for regular classes and functions. This makes it possible to accesss
this information for all dataclass parameters, especially those set as
`InitVar` where it was not possible to get it since they are not members
(attributes) of the class.

Issue mkdocstrings#286: mkdocstrings#286
Related to mkdocstrings#252: mkdocstrings#252
@has2k1
Copy link
Contributor Author

has2k1 commented Jun 10, 2024

@pawamoy, please kill this action.

When the test case for the encoder fails, the json strings being compared are so large they choke pytest when processing the error.

@has2k1
Copy link
Contributor Author

has2k1 commented Jun 10, 2024

It is ready review, the latest changes pass (for python<3.13) on my repo.

Copy link
Member

@pawamoy pawamoy left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Thanks a lot for the PR @has2k1!

So, with these changes, Parameters start to look a lot like Objects 🤔 I'm not sure that's something I want. That creates a bit of code duplication. We could reuse mixins to avoid that, but we don't want to go full in and make Parameters actual Objects, because they're not.

I see that you added these attributes/properties:

  • docstring
  • lineno
  • endlineno
  • parent
  • lines
  • linescollections
  • source

May I know why not just adding a docstring attribute (and parameter)?

To me, source will not be super useful: most of the time, multiple parameters appear on the same line. Even if we're OK with that, getting the source with these line numbers will most of the time again return code with invalid syntax (because the code will be incomplete, for example providing just the signature of a function, or even just a line of it). This is in contrast to the other objects, where their source property always return valid/complete code. I understand that this PR only gives docstrings to parameters of dataclasses, but since the API is there for any parameter, it must make sense for all of them.

If lineno, endlineno, lines and lines_collection were added to support source, then I think we can just get rid of all of them. I prefer not providing API than providing incorrect one. We can always revise later.

The parent attribute is probably interesting to have, just for navigation purposes (going from parameter to the function above). But then, lets keep things simple for now, and lets avoid giving Parameter too much of the shape of Objects: lets remove the module and filepath properties too. They can be accessed by passing through the parent first.

Finally, I wonder if using actual Docstrings makes sense: I don't think it makes sense to use docstring sections, or a particular docstring style for parameter docstrings? They usually are plain markup strings, no? Then, maybe parameter docstrings should just be strings? Ah, they could use generic admonitions which should be parsed too.

@has2k1
Copy link
Contributor Author

has2k1 commented Jun 11, 2024

You are right, the docstring is main information.

May I know why not just adding a docstring attribute (and parameter)

Using the source lines is a cheap hack to get nicely formatted code snippets, especially to deal with multiline annotations.

As these values are not available for non-dataclass parameters, it does make sense to limit the scope of what is added in this change.

Copy link
Member

@pawamoy pawamoy left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Thanks, it's super clean now :)

Just a few suggestions.

src/griffe/dataclasses.py Outdated Show resolved Hide resolved
src/griffe/dataclasses.py Outdated Show resolved Hide resolved
src/griffe/dataclasses.py Outdated Show resolved Hide resolved
Copy link
Member

@pawamoy pawamoy left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Thanks 🙂 I'll just rename parent to function and I think we're good.

src/griffe/dataclasses.py Outdated Show resolved Hide resolved
src/griffe/dataclasses.py Outdated Show resolved Hide resolved
@pawamoy pawamoy merged commit e21eabe into mkdocstrings:main Jun 12, 2024
22 of 26 checks passed
@pawamoy
Copy link
Member

pawamoy commented Jun 12, 2024

Squashed and merged, thanks again!

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

2 participants