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.

Sign up for GitHub

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

Jump to bottom

Document python requirement for tox/mypy & remove basepython from conf #2644

Merged
merged 6 commits into from Feb 18, 2022
Merged

Document python requirement for tox/mypy & remove basepython from conf #2644

Show file tree
Hide file tree
Changes from 3 commits
Commits
Show all changes
6 commits
Select commit Hold shift + click to select a range
853edf7
Pin python version for pre-commit hooks
juhoautio Feb 14, 2022
2979f5d
Doc python req for tox, remove basepython
juhoautio Feb 17, 2022
fe5e982
Merge branch 'main' into fix_precommit_mypy
juhoautio Feb 17, 2022
03df510
Update CONTRIBUTING.md
juhoautio Feb 17, 2022
b604536
Update CONTRIBUTING.md
juhoautio Feb 17, 2022
7e95f11
Merge branch 'main' into fix_precommit_mypy
tunetheweb Feb 18, 2022
File filter

Filter by extension

Filter by extension
Conversations
Failed to load comments.
Jump to
Jump to file
Failed to load files.
Diff view
Diff view
23 changes: 18 additions & 5 deletions CONTRIBUTING.md
View file
Open in desktop
Expand Up @@ -82,13 +82,20 @@ changes.

### Developing and Running SQLFluff Locally

#### Requirements

The simplest way to set up a development environment is to use `tox`.
First ensure that you have tox installed (windows users may have to replace `python3` with `py`):
First ensure that you have tox installed:
```shell
python3 -m pip install -U tox
python3.8 -m pip install -U tox
```
**IMPORTANT:** `tox` must be installed using Python 3.8 or newer. Otherwise `mypy` checks may fail. Depending on your setup and personal preference, replace `python3.8` as necessary, but make sure that the python version is new enough.
juhoautio marked this conversation as resolved.
Show resolved Hide resolved

<!-- Hopefully when tox 4 is released it will be possible to set `basepython = python3.10, python3.9, python3.8` to enforce a working version for running mypy checks. For now it's only possible to set it to a single version which is too restrictive. On the other hand for now there is a requirement for contributors to install tox with a new enough Python version. -->
juhoautio marked this conversation as resolved.
Show resolved Hide resolved

A virtual environment can then be created and activated by running:
#### Creating a virtual environment

A virtual environment can then be created and activated by running (check the [requirements](#requirements) before running this):
Copy link
Member

Choose a reason for hiding this comment

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

Not sure this is needed and reads a little confusing. I think the Then covers it.

Suggested change
A virtual environment can then be created and activated by running (check the [requirements](#requirements) before running this):
A virtual environment can then be created and activated by running:

Copy link
Contributor Author

Choose a reason for hiding this comment

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

I wanted to be extra sure. If someone just jumps to this section and happens to have tox installed previously, they might miss the requirement. Are you ok with keeping the pointer?

Copy link
Member

Choose a reason for hiding this comment

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

Yup. OK keeping them all as you prefer.

```shell
tox -e dbt021-py38 --devenv .venv
source .venv/bin/activate
Expand Down Expand Up @@ -130,7 +137,7 @@ pip install -e plugins/sqlfluff-templater-dbt/.

### Testing

To test locally, SQLFluff uses `tox`. The test suite can be run via:
To test locally, SQLFluff uses `tox` (check the [requirements](#requirements)!). The test suite can be run via:
Copy link
Member

Choose a reason for hiding this comment

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

Same feedback here. I know there’s a chance people can miss the early point but personally still find it a little overkill.

Suggested change
To test locally, SQLFluff uses `tox` (check the [requirements](#requirements)!). The test suite can be run via:
To test locally, SQLFluff uses `tox`. The test suite can be run via:


```shell
tox
Expand Down Expand Up @@ -209,7 +216,13 @@ py.test -v plugins/sqlfluff-templater-dbt/test/

### Pre-Commit Config

For development convenience we also provide a `.pre-commit-config.yaml` file to allow the user to install a selection of pre-commit hooks via `tox -e pre-commit -- install`. These hooks can help the user identify and fix potential linting/typing violations prior to committing their code and therefore reduce having to deal with these sort of issues during code review.
For development convenience we also provide a `.pre-commit-config.yaml` file to allow the user to install a selection of pre-commit hooks by running (check the [requirements](#requirements) before running this):
Copy link
Member

Choose a reason for hiding this comment

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

And here. Do think we’ve gone from too little guidance here to too much. But maybe you feel strongly about leaving some of these in place since I know you say you wasted a good bit of time on this?

Suggested change
For development convenience we also provide a `.pre-commit-config.yaml` file to allow the user to install a selection of pre-commit hooks by running (check the [requirements](#requirements) before running this):
For development convenience we also provide a `.pre-commit-config.yaml` file to allow the user to install a selection of pre-commit hooks by running:

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Out of the cases discussed, this is maybe the most important to keep the pointer. Because this is so far away from the main tox stuff, I can easily imagine someone jumping here just for the sake of installing pre-commit tools and again if that someone already has tox installed, it's not obvious that it could be wrong even though tox -e pre-commit -- install seems to succeed.

Copy link
Contributor Author

Choose a reason for hiding this comment

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

To make things worse, if you already ran this before you realized you need to change your tox installation, it's not trivial to realize that you also need to wipe out the .tox/pre-commit by hand and run tox -e pre-commit -- install again. Better safe than sorry, it's a pity that we can't enforce these programmatically.


```
tox -e pre-commit -- install
```

These hooks can help the user identify and fix potential linting/typing violations prior to committing their code and therefore reduce having to deal with these sort of issues during code review.

### Documentation Website

Expand Down
1 change: 0 additions & 1 deletion tox.ini
View file
Open in desktop
Expand Up @@ -94,7 +94,6 @@ deps =
commands = make -C {toxinidir}/docs html

[testenv:mypy]
basepython = python3.10
skip_install = true
commands = mypy {toxinidir}/src/sqlfluff

Expand Down