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

Fix singular and plural for "error(s)" #2157

Merged
merged 4 commits into from
Jan 25, 2023
Merged

Fix singular and plural for "error(s)" #2157

Show file tree
Hide file tree
Changes from all commits
Commits
Show all changes
4 commits
Select commit Hold shift + click to select a range
2bc0172
Fix singular and plural for error(s)
hugovk Jan 25, 2023
da18f5a
Allow testing feature branches
hugovk Jan 25, 2023
dfabc6e
Bump GitHub Actions
hugovk Jan 25, 2023
0973f35
Add colour to CI for readability
hugovk Jan 25, 2023
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
9 changes: 3 additions & 6 deletions .github/workflows/ci.yaml
View file
Open in desktop
Original file line number Diff line number Diff line change
@@ -1,10 +1,6 @@
name: CI

on:
push:
branches: [main]
pull_request:
branches: [main]
on: [push, pull_request, workflow_dispatch]
Copy link
Contributor

@andersk andersk Jan 25, 2023 •
edited

Choose a reason for hiding this comment

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

The issue with this configuration is that it always results in duplicate CI runs for pull requests: once for the push event in the head repository and once for the pull_request event in the base repository.

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Yes, I think that's fine.

First, so contributors can see test results it on their forks first, to make sure it passes before proceeding; and then again for the base repo, so the test results can be seen by maintainers in the PR.

Copy link
Member

Choose a reason for hiding this comment

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

Hmm, yeah, this actually does seem non-ideal, since PRs in the main repo are now showing all the duplicate tasks. It'd be nice to at least get rid of those somehow.

Copy link
Contributor

@andersk andersk Jan 25, 2023 •
edited

Choose a reason for hiding this comment

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

I’d recommend

on:
  push:
    branches: [main]
  pull_request:
  workflow_dispatch:

and in the unusual case where a contributor wants to run CI on their fork before creating a pull request, workflow_dispatch allows them to trigger it manually.

Copy link
Member

Choose a reason for hiding this comment

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

Thank you.

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Hi!

workflow_dispatch is good for manual triggers, but as a contributor who wants to test my code on CI before opening PRs, the amount of clicks it requires to start each build is pretty tedious. And CI should be automatic not manual.

CIs are amazing things - you can automatically test and lint your code on all these different versions! We should encourage contributors to use them. But if they don't want to, that's fine, and GitHub Actions is normally disabled for forks.

As another approach, how does this config look?

    # We want to run on external PRs, but not on our own internal PRs as they'll be run
    # by the push to the branch.
    if: github.event_name == 'push' || github.event.pull_request.head.repo.full_name != github.repository

re: https://github.com/has2k1/plotnine/blob/f696c01f1602e422a29778c112005463992ed707/.github/workflows/testing.yml#L10-L12

Copy link
Contributor

Choose a reason for hiding this comment

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

I don't really see the problem with having to open a PR to run the CI ... you can open the PR as a draft and mark it as ready once the CI has passed if you care about that.

Copy link
Contributor Author

Choose a reason for hiding this comment

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

One reason is to avoid noise on the tracker, for example, there may be lots of commits/rebases etc as things progress.

Another option is to follow the lead of things like pytest and pre-commit, and run the CI for branches prefixed with test-me-:

on:
  push:
    branches: [main, test-me-*]

https://github.com/pytest-dev/pytest/blob/ca40380e99c2cdaab1d0c041f9f28cff37ef8ff9/.github/workflows/test.yml#L3-L16

https://github.com/pre-commit/pre-commit/blob/dd8e717ed6022209a2b0cecf5c75460eb60e548e/.github/workflows/main.yml#L3-L5

Copy link
Contributor

Choose a reason for hiding this comment

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

You could open a PR in your own fork.


concurrency:
group: ${{ github.workflow }}-${{ github.ref_name }}-${{ github.event.pull_request.number || github.sha }}
Expand All @@ -13,6 +9,7 @@ concurrency:
env:
CARGO_INCREMENTAL: 0
CARGO_NET_RETRY: 10
CARGO_TERM_COLOR: always
RUSTUP_MAX_RETRIES: 10

jobs:
Expand Down Expand Up @@ -153,7 +150,7 @@ jobs:
runs-on: ubuntu-latest
steps:
- name: Checkout Actions Repository
uses: actions/checkout@v2
uses: actions/checkout@v3

- name: Check spelling of file.txt
uses: crate-ci/typos@master
Expand Down
21 changes: 11 additions & 10 deletions .github/workflows/flake8-to-ruff.yaml
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -12,6 +12,7 @@ env:
PYTHON_VERSION: "3.7" # to build abi3 wheels
CARGO_INCREMENTAL: 0
CARGO_NET_RETRY: 10
CARGO_TERM_COLOR: always
RUSTUP_MAX_RETRIES: 10

jobs:
Expand All @@ -38,7 +39,7 @@ jobs:
run: |
pip install dist/${{ env.CRATE_NAME }}-*.whl --force-reinstall
- name: Upload wheels
uses: actions/upload-artifact@v2
uses: actions/upload-artifact@v3
with:
name: wheels
path: dist
Expand All @@ -64,7 +65,7 @@ jobs:
run: |
pip install dist/${{ env.CRATE_NAME }}-*universal2.whl --force-reinstall
- name: Upload wheels
uses: actions/upload-artifact@v2
uses: actions/upload-artifact@v3
with:
name: wheels
path: dist
Expand Down Expand Up @@ -96,7 +97,7 @@ jobs:
run: |
python -m pip install dist/${{ env.CRATE_NAME }}-*.whl --force-reinstall
- name: Upload wheels
uses: actions/upload-artifact@v2
uses: actions/upload-artifact@v3
with:
name: wheels
path: dist
Expand All @@ -123,7 +124,7 @@ jobs:
run: |
pip install dist/${{ env.CRATE_NAME }}-*.whl --force-reinstall
- name: Upload wheels
uses: actions/upload-artifact@v2
uses: actions/upload-artifact@v3
with:
name: wheels
path: dist
Expand All @@ -144,7 +145,7 @@ jobs:
target: ${{ matrix.target }}
manylinux: auto
args: --no-default-features --release --out dist -m ./${{ env.CRATE_NAME }}/Cargo.toml
- uses: uraimo/run-on-arch-action@v2.0.5
- uses: uraimo/run-on-arch-action@v2.5.0
if: matrix.target != 'ppc64'
name: Install built wheel
with:
Expand All @@ -158,7 +159,7 @@ jobs:
run: |
pip3 install ${{ env.PACKAGE_NAME }} --no-index --find-links dist/ --force-reinstall
- name: Upload wheels
uses: actions/upload-artifact@v2
uses: actions/upload-artifact@v3
with:
name: wheels
path: dist
Expand Down Expand Up @@ -192,7 +193,7 @@ jobs:
apk add py3-pip
pip3 install ${{ env.PACKAGE_NAME }} --no-index --find-links /io/dist/ --force-reinstall
- name: Upload wheels
uses: actions/upload-artifact@v2
uses: actions/upload-artifact@v3
with:
name: wheels
path: dist
Expand Down Expand Up @@ -228,7 +229,7 @@ jobs:
run: |
pip3 install ${{ env.PACKAGE_NAME }} --no-index --find-links dist/ --force-reinstall
- name: Upload wheels
uses: actions/upload-artifact@v2
uses: actions/upload-artifact@v3
with:
name: wheels
path: dist
Expand Down Expand Up @@ -262,7 +263,7 @@ jobs:
run: |
pip install dist/${{ env.CRATE_NAME }}-*.whl --force-reinstall
- name: Upload wheels
uses: actions/upload-artifact@v2
uses: actions/upload-artifact@v3
with:
name: wheels
path: dist
Expand All @@ -280,7 +281,7 @@ jobs:
- musllinux-cross
- pypy
steps:
- uses: actions/download-artifact@v2
- uses: actions/download-artifact@v3
with:
name: wheels
- uses: actions/setup-python@v4
Expand Down
1 change: 1 addition & 0 deletions .github/workflows/playground.yaml
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -8,6 +8,7 @@ on:
env:
CARGO_INCREMENTAL: 0
CARGO_NET_RETRY: 10
CARGO_TERM_COLOR: always
RUSTUP_MAX_RETRIES: 10

jobs:
Expand Down
21 changes: 11 additions & 10 deletions .github/workflows/ruff.yaml
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -13,6 +13,7 @@ env:
PYTHON_VERSION: "3.7" # to build abi3 wheels
CARGO_INCREMENTAL: 0
CARGO_NET_RETRY: 10
CARGO_TERM_COLOR: always
RUSTUP_MAX_RETRIES: 10

jobs:
Expand All @@ -39,7 +40,7 @@ jobs:
run: |
pip install dist/${{ env.PACKAGE_NAME }}-*.whl --force-reinstall
- name: Upload wheels
uses: actions/upload-artifact@v2
uses: actions/upload-artifact@v3
with:
name: wheels
path: dist
Expand All @@ -65,7 +66,7 @@ jobs:
run: |
pip install dist/${{ env.PACKAGE_NAME }}-*universal2.whl --force-reinstall
- name: Upload wheels
uses: actions/upload-artifact@v2
uses: actions/upload-artifact@v3
with:
name: wheels
path: dist
Expand Down Expand Up @@ -97,7 +98,7 @@ jobs:
run: |
python -m pip install dist/${{ env.PACKAGE_NAME }}-*.whl --force-reinstall
- name: Upload wheels
uses: actions/upload-artifact@v2
uses: actions/upload-artifact@v3
with:
name: wheels
path: dist
Expand All @@ -124,7 +125,7 @@ jobs:
run: |
pip install dist/${{ env.PACKAGE_NAME }}-*.whl --force-reinstall
- name: Upload wheels
uses: actions/upload-artifact@v2
uses: actions/upload-artifact@v3
with:
name: wheels
path: dist
Expand All @@ -145,7 +146,7 @@ jobs:
target: ${{ matrix.target }}
manylinux: auto
args: --no-default-features --release --out dist
- uses: uraimo/run-on-arch-action@v2.0.5
- uses: uraimo/run-on-arch-action@v2.5.0
if: matrix.target != 'ppc64'
name: Install built wheel
with:
Expand All @@ -159,7 +160,7 @@ jobs:
run: |
pip3 install ${{ env.PACKAGE_NAME }} --no-index --find-links dist/ --force-reinstall
- name: Upload wheels
uses: actions/upload-artifact@v2
uses: actions/upload-artifact@v3
with:
name: wheels
path: dist
Expand Down Expand Up @@ -193,7 +194,7 @@ jobs:
apk add py3-pip
pip3 install ${{ env.PACKAGE_NAME }} --no-index --find-links /io/dist/ --force-reinstall
- name: Upload wheels
uses: actions/upload-artifact@v2
uses: actions/upload-artifact@v3
with:
name: wheels
path: dist
Expand Down Expand Up @@ -229,7 +230,7 @@ jobs:
run: |
pip3 install ${{ env.PACKAGE_NAME }} --no-index --find-links dist/ --force-reinstall
- name: Upload wheels
uses: actions/upload-artifact@v2
uses: actions/upload-artifact@v3
with:
name: wheels
path: dist
Expand Down Expand Up @@ -263,7 +264,7 @@ jobs:
run: |
pip install dist/${{ env.PACKAGE_NAME }}-*.whl --force-reinstall
- name: Upload wheels
uses: actions/upload-artifact@v2
uses: actions/upload-artifact@v3
with:
name: wheels
path: dist
Expand All @@ -282,7 +283,7 @@ jobs:
- pypy
if: "startsWith(github.ref, 'refs/tags/')"
steps:
- uses: actions/download-artifact@v2
- uses: actions/download-artifact@v3
with:
name: wheels
- uses: actions/setup-python@v4
Expand Down
2 changes: 1 addition & 1 deletion README.md
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -1696,7 +1696,7 @@ After installing `ruff` and `nbqa`, you can run Ruff over a notebook like so:
Untitled.ipynb:cell_1:2:5: F841 Local variable `x` is assigned to but never used
Untitled.ipynb:cell_2:1:1: E402 Module level import not at top of file
Untitled.ipynb:cell_2:1:8: F401 `os` imported but unused
Found 3 error(s).
Found 3 errors.
1 potentially fixable with the --fix option.
```

Expand Down
12 changes: 6 additions & 6 deletions resources/test/project/README.md
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -16,7 +16,7 @@ resources/test/project/examples/docs/docs/file.py:1:1: I001 Import block is un-s
resources/test/project/examples/docs/docs/file.py:8:5: F841 Local variable `x` is assigned to but never used
resources/test/project/project/file.py:1:8: F401 `os` imported but unused
resources/test/project/project/import_file.py:1:1: I001 Import block is un-sorted or un-formatted
Found 7 error(s).
Found 7 errors.
7 potentially fixable with the --fix option.
```

Expand All @@ -31,7 +31,7 @@ examples/docs/docs/file.py:1:1: I001 Import block is un-sorted or un-formatted
examples/docs/docs/file.py:8:5: F841 Local variable `x` is assigned to but never used
project/file.py:1:8: F401 `os` imported but unused
project/import_file.py:1:1: I001 Import block is un-sorted or un-formatted
Found 7 error(s).
Found 7 errors.
7 potentially fixable with the --fix option.
```

Expand All @@ -42,7 +42,7 @@ files:
∴ (cd resources/test/project/examples/docs && cargo run .)
docs/file.py:1:1: I001 Import block is un-sorted or un-formatted
docs/file.py:8:5: F841 Local variable `x` is assigned to but never used
Found 2 error(s).
Found 2 errors.
2 potentially fixable with the --fix option.
```

Expand All @@ -60,7 +60,7 @@ resources/test/project/examples/docs/docs/file.py:3:8: F401 `numpy` imported but
resources/test/project/examples/docs/docs/file.py:4:27: F401 `docs.concepts.file` imported but unused
resources/test/project/examples/excluded/script.py:1:8: F401 `os` imported but unused
resources/test/project/project/file.py:1:8: F401 `os` imported but unused
Found 9 error(s).
Found 9 errors.
9 potentially fixable with the --fix option.
```

Expand All @@ -73,7 +73,7 @@ docs/docs/concepts/file.py:5:5: F841 Local variable `x` is assigned to but never
docs/docs/file.py:1:1: I001 Import block is un-sorted or un-formatted
docs/docs/file.py:8:5: F841 Local variable `x` is assigned to but never used
excluded/script.py:5:5: F841 Local variable `x` is assigned to but never used
Found 4 error(s).
Found 4 errors.
4 potentially fixable with the --fix option.
```

Expand All @@ -82,7 +82,7 @@ Passing an excluded directory directly should report errors in the contained fil
```
∴ cargo run resources/test/project/examples/excluded/
resources/test/project/examples/excluded/script.py:1:8: F401 `os` imported but unused
Found 1 error(s).
Found 1 error.
1 potentially fixable with the --fix option.
```

Expand Down
18 changes: 13 additions & 5 deletions ruff_cli/src/printer.rs
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -96,12 +96,14 @@ impl<'a> Printer<'a> {
let remaining = diagnostics.messages.len();
let total = fixed + remaining;
if fixed > 0 {
let s = if total == 1 { "" } else { "s" };
writeln!(
stdout,
"Found {total} error(s) ({fixed} fixed, {remaining} remaining)."
"Found {total} error{s}) ({fixed} fixed, {remaining} remaining)."
Copy link
Contributor

Choose a reason for hiding this comment

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

)?;
} else if remaining > 0 {
writeln!(stdout, "Found {remaining} error(s).")?;
let s = if remaining == 1 { "" } else { "s" };
writeln!(stdout, "Found {remaining} error{s}.")?;
}

if !matches!(self.autofix, fix::FixMode::Apply) {
Expand All @@ -121,10 +123,11 @@ impl<'a> Printer<'a> {
Violations::Hide => {
let fixed = diagnostics.fixed;
if fixed > 0 {
let s = if fixed == 1 { "" } else { "s" };
if matches!(self.autofix, fix::FixMode::Apply) {
writeln!(stdout, "Fixed {fixed} error(s).")?;
writeln!(stdout, "Fixed {fixed} error{s}.")?;
} else if matches!(self.autofix, fix::FixMode::Diff) {
writeln!(stdout, "Would fix {fixed} error(s).")?;
writeln!(stdout, "Would fix {fixed} error{s}.")?;
}
}
}
Expand Down Expand Up @@ -339,8 +342,13 @@ impl<'a> Printer<'a> {
}

if self.log_level >= &LogLevel::Default {
let s = if diagnostics.messages.len() == 1 {
""
} else {
"s"
};
notify_user!(
"Found {} error(s). Watching for file changes.",
"Found {} error{s}. Watching for file changes.",
diagnostics.messages.len()
);
}
Expand Down
6 changes: 3 additions & 3 deletions ruff_cli/tests/integration_test.rs
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -28,7 +28,7 @@ fn test_stdin_error() -> Result<()> {
.failure();
assert_eq!(
str::from_utf8(&output.get_output().stdout)?,
"-:1:8: F401 `os` imported but unused\nFound 1 error(s).\n1 potentially fixable with the \
"-:1:8: F401 `os` imported but unused\nFound 1 error.\n1 potentially fixable with the \
--fix option.\n"
);
Ok(())
Expand All @@ -44,8 +44,8 @@ fn test_stdin_filename() -> Result<()> {
.failure();
assert_eq!(
str::from_utf8(&output.get_output().stdout)?,
"F401.py:1:8: F401 `os` imported but unused\nFound 1 error(s).\n1 potentially fixable \
with the --fix option.\n"
"F401.py:1:8: F401 `os` imported but unused\nFound 1 error.\n1 potentially fixable with \
the --fix option.\n"
);
Ok(())
}
Expand Down