fix(testing+linting): add nox lint+format directives #123
Conversation
product-auto-label
bot
added
the
api: spanner
|
@Yuan325, Can you also review it and run the tests? |
odeke-em
force-pushed
the
testing-add-nox-lint+blacken+enable-unit-testing
branch
from
af7b616 to
312898d
Compare
There was a problem hiding this comment.
Please remove these changes. This project (and consistently across other projects) does not use nox to run lints. Please see https://github.com/googleapis/langchain-google-spanner-python/blob/main/.github/workflows/lint.yml
There was a problem hiding this comment.
Hi @averikitsch, Do we have a reason for not using it?
There was a problem hiding this comment.
- The dependencies are in the pyproject.toml that get updated by dependabot. This file does not get automatically updated
- This config is copied from repos that use a different package naming schema, different directory structure, and setup.py
Also, for the future we should not be using "fix" in the commit message, that will trigger a new release and we do not create new releases for CI updates.
There was a problem hiding this comment.
@averikitsch thanks for your reply. The purpose of the PR is to get functionality to allow code to be formatted, unit tests to be run locally et al as code is being built by developers so as to speed up development. Do you perhaps have alternative suggestions for how to accomplish the above?
There was a problem hiding this comment.
Please remove these changes. This project (and consistently across other projects) does not use nox to run lints.
@averikitsch python-spanner the main repo representing Cloud Spanner does use nox https://github.com/googleapis/python-spanner/blob/main/noxfile.py for linting, testing and all, for more than 7 years https://github.com/googleapis/python-spanner/commits/main/noxfile.py?after=32e761b0d4052938bf67cfec63a0e83702a35ada+34.
Could you perhaps explain your objections and help out with suggestions? This PR has been sitting for more than 2 weeks and it moving is necessary to accomplish other tasks.
There was a problem hiding this comment.
Hi @averikitsch,
@odeke-em is trying to define a nox task for liniting and running the test locally and for linting he is using black (i.e. we are using same in our workflows).
If noxfile.py is not getting copied from any other repos or configurations.. I don't see any problem with the change.
There was a problem hiding this comment.
This will be diverting from the other langchain repos. I am fine with adding it, but the Spanner team will have to keep it up to date as I mentioned that it doesn't get auto-updates and will not stay in sync with the repo CI setup. This may cause long term confusion for maintainers.
There was a problem hiding this comment.
Thank you @averikitsch. It won't need many updates except indeed as you mention mirror-ing pyproject dependencies into setup.py and those won't be updates needed even with a year. However, the benefits of having non-Googlers be able to develop and make updates robustly I believe shall heavily outweigh any of those. Also currently I can't even look at the Kokoro statuses as one needs to be a Googler per https://console.cloud.google.com/cloud-build/triggers/edit/16ba25ff-d74a-42ef-8f19-9780571439ab?project=585680705374
There was a problem hiding this comment.
- The test dependencies get updated more than yearly via automation. Therefore will be out of sync of the CI system that is getting periodic updates which may cause confusion.
- This Python library does not use setup.py, it uses a pyproject.toml (https://packaging.python.org/en/latest/guides/modernize-setup-py-project/#modernize-setup-py-project).
- I am unsure how this relates to permissions for accessing CI tests. You can run the tests locally using
pytestas noted in https://github.com/googleapis/langchain-google-spanner-python/blob/main/DEVELOPER.md#testing. Using the noxfile is just wrapping that command.
|
Please fix the lint errors |
|
/gcbrun |
|
Please work with @gauravpurohit06 to run the tests via the comment "/gcbrun" |
odeke-em
force-pushed
the
testing-add-nox-lint+blacken+enable-unit-testing
branch
from
312898d to
b0a4104
Compare
|
/gcbrun |
|
Please fix the linting errors. |
|
/gcbrun |
|
@gauravpurohit06 kindly help me run /gcbrun |
|
/gcbrun |
|
/gcbrun |
odeke-em
force-pushed
the
testing-add-nox-lint+blacken+enable-unit-testing
branch
from
fc0b553 to
bf7e000
Compare
|
@gauravpurohit06 kindly help with /gcbrun |
|
/gcbrun |
This change introduces new nox directives: * blacken: `nox -s blacken` * format: `nox -s format` to apply formatting to files * lint: `nox -s lint` to flag linting issues * integration: to run integration tests * unit: to run unit tests locally which are the basis to enable scalable development and continuous testing as I prepare to bring in Approximate Nearest Neighors (ANN) functionality into this package.
odeke-em
force-pushed
the
testing-add-nox-lint+blacken+enable-unit-testing
branch
from
bf7e000 to
50d0cfc
Compare
odeke-em
force-pushed
the
testing-add-nox-lint+blacken+enable-unit-testing
branch
from
50d0cfc to
e1b166d
Compare
|
@gauravpurohit06 kindly please help with /gcbrun |
|
/gcbrun |
This change introduces new nox directives:
nox -s blackennox -s formatto apply formatting to filesnox -s lintto flag linting issueswhich are the basis to enable scalable development and continuous testing as I prepare to bring in
Approximate Nearest Neighors (ANN) functionality into this package.