Skip to content
This repository was archived by the owner on Jan 19, 2025. It is now read-only.

Conversation

Aclrian
Copy link
Contributor

@Aclrian Aclrian commented Nov 7, 2022

Closes #1106.

Summary of Changes

added an attribute code to functions and classes that contain the unformatted source code of the function or class

Testing Instructions

run package-parser api -p sklearn -o ./out and compare the output with the original file that the element contains.
package-parser annotations -a ../data/api/sklearn__api.json -u ../data/usages/sklearn_usages_counts.json -o ./out/annotations.json can verify that no error occur if the file is loaded.

@Aclrian Aclrian linked an issue Nov 7, 2022 that may be closed by this pull request
@github-actions
Copy link

github-actions bot commented Nov 7, 2022

🦙 MegaLinter status: ✅ SUCCESS

Descriptor Linter Files Fixed Errors Elapsed time
✅ JSON eslint-plugin-jsonc 1 0 0 9.24s
✅ JSON jsonlint 1 0 0.57s
✅ JSON prettier 1 0 0 7.27s
✅ JSON v8r 1 0 1.96s
✅ PYTHON bandit 3 0 0.76s
✅ PYTHON black 3 0 0 1.42s
✅ PYTHON flake8 3 0 0.58s
✅ PYTHON isort 3 0 0 1.27s
✅ PYTHON mypy 3 0 2.1s
✅ PYTHON pylint 3 0 4.45s

See errors details in artifact MegaLinter reports on CI Job page
Set VALIDATE_ALL_CODEBASE: true in mega-linter.yml to validate all sources, not only the diff

MegaLinter is graciously provided by OX Security

@Aclrian
Copy link
Contributor Author

Aclrian commented Nov 7, 2022

I have two questions:

@Aclrian Aclrian marked this pull request as ready for review November 7, 2022 13:05
@Aclrian Aclrian requested a review from lars-reimann as a code owner November 7, 2022 13:05
@lars-reimann
Copy link
Member

lars-reimann commented Nov 7, 2022

  • Does it need tests?

A function like trim_code should have tests. However, in this case you might be able to simplify the code by using the as_string method of astroid. And if you're only calling a well-tested third-party function, you don't need tests.

For the sake of easy (manual) testing, it makes sense to update the data folder. Just include the license file from scikit-learn as well in the data/api folder.

@Aclrian
Copy link
Contributor Author

Aclrian commented Nov 8, 2022

However, in this case you might be able to simplify the code by using the as_string method of astroid.

I compared output of the alternatives and found out that the as_string method ignores comments in the code, but it includes decorators. With a change from fromlineno to lineno decorators would be visible at the current process. Also, it seems that as_string() rearrange lines by removing comments.

Original sklearn.datasets._base.load_boston's last lines:

    return Bunch(
        data=data,
        target=target,
        # last column is target value
        feature_names=feature_names[:-1],
        DESCR=descr_text,
        filename=data_file_name,
        data_module=DATA_MODULE,
    )

and what as_string() converts it to:

    return Bunch(data=data, target=target, feature_names=feature_names[:-1], DESCR=descr_text, filename=data_file_name, data_module=DATA_MODULE)

If the comment line were removed in a new version, there would be a few new lines when we change to as_string().
So, I think we should keep the current procedure.

@lars-reimann
Copy link
Member

Let's discuss this tomorrow.

@Aclrian Aclrian force-pushed the 1106-api-command-should-extract-source-code-of-api-elements branch from 10744bd to a05685a Compare November 9, 2022 10:49
@Aclrian Aclrian requested a review from lars-reimann November 10, 2022 18:24
@lars-reimann
Copy link
Member

Please resolve conversations yourself once you are done with them.

@lars-reimann lars-reimann merged commit b79634d into main Nov 16, 2022
@lars-reimann lars-reimann deleted the 1106-api-command-should-extract-source-code-of-api-elements branch November 16, 2022 12:50
@github-actions
Copy link

🎉 This PR is included in version 1.67.0 🎉

The release is available on GitHub release

Your semantic-release bot 📦🚀

@github-actions github-actions bot added the released Included in a release label Nov 16, 2022
Sign up for free to subscribe to this conversation on GitHub. Already have an account? Sign in.

Labels

released Included in a release

Projects

None yet

Development

Successfully merging this pull request may close these issues.

api command should extract source code of API elements

2 participants