robotframework / robotframework

Generic automation framework for acceptance testing and RPA
http://robotframework.org
Apache License 2.0
9.8k stars 2.33k forks source link

`robot.api.parsing` doesn't have properly defined public API #5245

Open martinezlc99 opened 5 days ago

martinezlc99 commented 5 days ago

Following the examples in the documentation causes Pylance to report reportPrivateImportUsage for any exported symbol from robot.api.parsing. There are no errors in execution, just the Pylance error.

Not sure this is an issue in Robot Framework per se, but removing the parenthesis around the parsing symbols removes the error.

For example, updating this:

from robot.parsing import (get_tokens, get_resource_tokens, get_init_tokens,
                           get_model, get_resource_model, get_init_model, Token)

to

from robot.parsing import get_tokens, get_resource_tokens, get_init_tokens, get_model, get_resource_model, get_init_model, Token

removes the error.

Glad to submit a PR if we want to address in Robot Framework for now.

Robot Framework version 7.1 Pylance extension version 2024.10.1 python version 3.12.7 macos Sequoia 15.0.1

pekkaklarck commented 5 days ago

I guess the problem is that what's considered a public API isn't well-defined in Python and different tools handle that differently:

I'd be fine with defining public APIs better, but I don't think removing parentheses is the right way. If you want to help get this happen, please study what the alternatives are and how different tools handle them. The tool I'm personally most interested in is mkdocstrings, because we use that for API doc generation with the new Manual that is developed at https://pekkaklarck.github.io/manual/.

martinezlc99 commented 4 days ago

Hi @pekkaklarck - thank you for the quick response.

Looking into this more, I believe the issue is actually your second bullet point. Discussion from my question at pyright project:

This package is a "py.typed" library, which means standard-compliant type checkers like pyright follow these rules when determining whether a symbol is public or private. In this case, the get_model symbol is not publicly exported from the robot.api.parsing submodule. If the library authors had intended for it to be publicly exported from this submodule, they would have needed to use a redundant import form (from robot.parsing import get_model as get_model) or include "get_model" in an all definition.

So the apparent fix is to update robot.api.parsing to export the symbols publically:

from robot.parsing import (
    get_tokens as get_tokens,
    get_resource_tokens as get_resource_tokens,
    get_init_tokens as get_init_tokens,
    get_model as get_model,
    get_resource_model as get_resource_model,
    get_init_model as get_init_model,
    Token as Token,
)

# same for other imports in this file

i don't think this will impact doc generation and I was able to successfully build the docs following instructions from BUILD.rst. I believe this will also be the case for mkdocstrings, although there are some discussions on this topic - see mkdocstrings/python#167 and mkdocstrings/mkdocstrings#656

Importing symbols from other modules in robot.api make pyright happy, although import of class symbols will also need to be updated:

# pyright: reportPrivateImportUsage=true
from robot.api import TestCase # reportPrivateImportUsage Error

Other methods of importing of the parsing API appear to work fine:

# pyright: reportPrivateImportUsage=true
from robot.api import parsing

m = parsing.get_model("robot.robot") # ok

or even

# pyright: reportPrivateImportUsage=true
import robot.api.parsing

m = robot.api.parsing.get_model("robot.robot) # ok

So this would only address the case of importing the symbol directly per the documentation examples.

Glad to send a PR if you would accept.

pekkaklarck commented 4 days ago

Thanks for digging deeper! I didn't know about the explicit rules related to the module interface you had found. As I wrote earlier, I knew about __all__ and the import x as x idiom, but didn't know them being standardized in the typing context. As I also wrote, I don't like maintaining __all__, so I'd say we should use the somewhat strange but at least easy-to-maintain redundant import alias approach.

Please submit a PR. Notice that there are some imports in robot.api.interfaces that probably should get this treatment as well. It could also be considered with robot.running and robot.result, because that's where you need to import things like TestCase and While. These modules aren't yet py.typed, so that may be a reason there are no warnings. Anyway, it's probably best to handle robot.api first and then later look at other modules.

pekkaklarck commented 4 days ago

Hmm, I quickly tested with mkdocstrings and it seems to support only __all__ and not the redundant import idiom. I'll ask about this on their gitter tomorrow unless you want to do that earlier. Let's anyway wait what they say before deciding which approach to use. I've discussed with them few times lately and they are very reasonable. If we point them to the typing spec, there's a good change they are willing to add support to the redundant import idiom.

pekkaklarck commented 4 days ago

If you want to test with mkdocstrings yourself, clone the https://github.com/pekkaklarck/manual project, install dependencies with pip install -r requirements.txt (a virtual environment is a good idea), make changes to code under src/robot, run mkdocs serve, and see changes at http://127.0.0.1:8000.

Actual PR to specify the public API should be done to this repository. Code in the Manual repository is there just to see how API docs are generated. Most of the modules still use reStructuredText and they'll be converted to Markdown only after the Manual is migrated to this repository.

martinezlc99 commented 4 days ago

Thank you @pekkaklarck for the quick response.

I'll ask about this on their gitter tomorrow unless you want to do that earlier. Let's anyway wait what they say before deciding which approach to use. I've discussed with them few times lately and they are very reasonable.

I'll standby for your direction after discussing with them as you have recent communication with them.

If you want to test with mkdocstrings yourself, clone the https://github.com/pekkaklarck/manual project, install dependencies with pip install -r requirements.txt (a virtual environment is a good idea), make changes to code under src/robot, run mkdocs serve, and see changes at http://127.0.0.1:8000/.

I'll definitely checkout mkdocstrings. Actually at my job, we are transitioning to material for mkdocs, but mkdocsforstrings is new to me and seems very interesting!

Again, appreciate your assistance!

martinezlc99 commented 4 days ago

Oh and I noticed my previous assertion of other methods of importing of the parsing API appearing to work fine was incorrect. That is, all the below are indeed pyright reportPrivateImportUsage errors

# pyright: reportPrivateImportUsage=true
from robot.api import parsing # ok

m = parsing.get_model("robot.robot") # but reportPrivateImportUsage error here
# pyright: reportPrivateImportUsage=true
import robot.api.parsing # ok

m = robot.api.parsing.get_model("robot.robot) # but reportPrivateImportUsage error here

Believe the "fix" is this same, just clarifying my previous statement.

pekkaklarck commented 4 days ago

I asked about this and got a reply. At the moment mkdocstrings supports only __all__ natively, but the redundant alias approach is supported by a dedicated plugin that's currently only available for sponsors.

I believe using redundant aliases is fine for us. We then have these options with the API docs:

  1. Become mkdocstrings sponsor. We may want to do that anyway, then there's still some effort setting, for example, GitHub Actions so that we get the access to private features.
  2. Wait for the redundant alias functionality to be released publicly. Code will work but API docs are lacking. Not ideal, unless the feature is released in the near future.
  3. Mention exported functions and classes in the module docstring. We actually already do this with Sphinx, so the situation wouldn't change. We just need to convert API docs from reStructuredText to Markdown, but we need to do that anyway when moving to MkDocs and mkdocstrings.

I'll decide between options 1. and 3. when we actually convert API docs. Regardless the choice, we can use redundant aliases now.

martinezlc99 commented 4 days ago

@pekkaklarck - just sent a PR for this. I did go beyond just the robot.api.parsing module, and updated all public symbols in robot.api. Please let me know if you want only robot.api.parsing updated.