-
Notifications
You must be signed in to change notification settings - Fork 1.8k
[flake8-pyi] Avoid syntax error from conflict with PIE790 (PYI021)
#20010
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.
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
Merged
Merged
Changes from all commits
Commits
Show all changes
203 commits
Select commit
Hold shift + click to select a range
db6fe35
[ruff] #19982 add isolation rule for PYI021
second-ed a50b389
[flake8-pyi] add new test for isolation check for rule (PYI021)
second-ed 3264d63
[flake8-pyi] isolation level hardcode for poc (PYI021)
second-ed 8af4168
[flake8-pyi] revert body_len variable (PYI021)
second-ed 7800db2
feat: statement_id method
second-ed f849dd7
[flake8-pyi] move PYI021 check to ast stage (PYI021)
second-ed d068e13
[flake8-pyi] use docstring_from fn (PYI021)
second-ed d5a2e80
[`pyupgrade`] Avoid reporting `__future__` features as unnecessary wh…
IDrokin117 7888e0c
[ty] add docstrings to completions based on type (#20008)
Gankra 84db505
[ty] Add link for namespaces being partial (#20015)
Gankra ebcf493
[ty] Avoid unnecessary argument type expansion (#19999)
dhruvmanila be4823f
Fix rust feature activation (#20012)
ntBre cfff126
[ty] Use `dedent` in cursor tests (#20019)
MichaReiser 91b4241
[ty] Perform assignability etc checks using new `Constraints` trait (…
dcreager 5354fac
Move diff rendering to `ruff_db` (#20006)
ntBre 8abb95d
[ty] Stop running every mdtest twice
BurntSushi 94040a9
Bump 0.12.10 (#20025)
dylwil3 34b218e
[ty] fix GotoTargets for keyword args in nested function calls (#20013)
Gankra e44bb15
[ty] Fix incorrect docstring in call signature completion (#20021)
MichaReiser a363bb8
[ty] Improve diagnostics for bad calls to functions (#20022)
AlexWaygood 6c5719e
[ty] Sync vendored typeshed stubs (#20031)
github-actions[bot] 15a3739
[ty] Cancel background tasks when shutdown is requested (#20039)
MichaReiser 6876231
[ty] Close signature help after `)` (#20017)
MichaReiser 5d95dcd
[`ruff`] Fix false positive for t-strings in `default-factory-kwarg` …
maxmynter 43d7463
[ty] rename BareTypeAliasType to ManualPEP695TypeAliasType (#20037)
carljm 6e68f91
Fix incorrect lsp inlay hint type (#20044)
MatthewMckee4 c2a80b9
[`ruff`] Handle empty t-strings in `unnecessary-empty-iterable-within…
dylwil3 d12f57c
[ty] Add type as detail to completion items (#20047)
MichaReiser df57afa
[`flake8-use-pathlib`] Add autofix for `PTH211` (#20009)
chirizxc 0a35b03
Add testing helper to compare stable vs preview snapshots (#19715)
vivster7 82eb353
[ty] Disallow std::env and io methods in most ty crates (#20046)
MichaReiser ac8784e
[ty] Add precise iteration and unpacking inference for string literal…
AlexWaygood 31de0dd
[`flake8-use-pathlib`] Fix `PTH211` autofix (#20049)
chirizxc 6768657
[ty] Shrink size of `AstNodeRef` (#20028)
ibraheemdev 1ecabd1
[ty] Remove duplicate global lint registry (#20053)
ibraheemdev 789f38a
[ty] Add a TODO for linting on `todo!`
BurntSushi 2dbc45a
[ty] Add debug trace for workspace symbol elapsed time
BurntSushi 03be2ed
[ty] Move query filtering outside of symbol visitor
BurntSushi 02044a5
[ty] Add some unit tests for "query matches symbol"
BurntSushi a722c40
[ty] Optimize query string matching
BurntSushi f378db7
[ty] Optimize "workspace symbols" retrieval
BurntSushi 7e84da5
[ty] Parallelize workspace symbols
BurntSushi de2fc0a
[ty] Rejigger workspace symbols for more efficient caching
BurntSushi 883cc23
[ty] Lightly refactor document symbols AST visitor
BurntSushi 218684c
[ty] Add Top[] and Bottom[] special forms, replacing top_materializat…
JelleZijlstra 85dea0e
Update Rust crate filetime to v0.2.26 (#20064)
renovate[bot] e2a218a
Update dependency ruff to v0.12.10 (#20062)
renovate[bot] 4c19e05
Update Rust crate bitflags to v2.9.3 (#20063)
renovate[bot] adf3e7b
Update Rust crate ordermap to v0.5.9 (#20065)
renovate[bot] 5cece85
Update Rust crate proc-macro2 to v1.0.101 (#20066)
renovate[bot] 7a142ad
Update Rust crate serde_json to v1.0.143 (#20069)
renovate[bot] e322e6e
Update astral-sh/setup-uv action to v6.6.0 (#20074)
renovate[bot] ef666dc
Update Rust crate thiserror to v2.0.16 (#20071)
renovate[bot] 46dbabd
Update Rust crate tracing-indicatif to v0.3.13 (#20072)
renovate[bot] a7e1390
Update Rust crate syn to v2.0.106 (#20070)
renovate[bot] d4cb5dd
Update Rust crate url to v2.5.7 (#20073)
renovate[bot] e8ca459
Update cargo-bins/cargo-binstall action to v1.15.1 (#20075)
renovate[bot] d11d9cc
Update Rust crate regex to v1.11.2 (#20067)
renovate[bot] b6d0865
Update Rust crate regex-automata to v0.4.10 (#20068)
renovate[bot] 51e8324
Update actions/checkout digest to ff7abcd (#20061)
renovate[bot] a4b9b18
[ty] Limit argument expansion size for overload call evaluation (#20041)
dhruvmanila ddacc6f
[ty] validate constructor call of `TypedDict` (#19810)
PrettyWood a136e54
Improve diff rendering for notebooks (#20036)
ntBre 6bc40d0
[ty] Completely ignore typeshed's stub for `Any` (#20079)
AlexWaygood 6dc4233
[`airflow`] replace wrong path `airflow.io.stroage` as `airflow.io.st…
Lee-W 16b34a9
[ty] Sync vendored typeshed stubs (#20083)
github-actions[bot] 153d3c6
[ty] don't mark entire type-alias scopes as Deferred (#20086)
carljm a3fc78a
[ty] Add support for PEP 800 (#20084)
AlexWaygood 81c0f9d
[ty] Add support for PEP 750 t-strings (#20085)
dylwil3 711de3c
[ty] Refactor inlay hints structure to use separate parts (#20052)
MatthewMckee4 ef10020
Fix incorrect D413 links in docstrings convention FAQ (#20089)
Avasam 3cc1e24
Add a `ScopeKind` for the `__class__` cell (#20048)
ntBre a8d4758
[`flake8-logging-format`] Add auto-fix for f-string logging calls (`G…
hamirmahal fc140ce
[ty] Add search paths info to unresolved import diagnostics (#20040)
Renkai db1f50e
[`flake8-use-pathlib`] Delete unused `Rule::OsSymlink` enabled check …
chirizxc 61eabcc
[`flake8-use-pathlib`] Make `PTH100` fix unsafe because it can change…
chirizxc e5651ed
[`flake8-use-pathlib`] Update links to the table showing the correspo…
chirizxc abb215b
[ty] don't eagerly unpack aliases in user-authored unions (#20055)
carljm 6dcca17
[ty] Add more tests for protocols (#20095)
AlexWaygood 09fb798
[ty] Infer slightly more precise types for comprehensions (#20111)
AlexWaygood a867935
[ty] Fix the inferred interface of specialized generic protocols (#19…
AlexWaygood 8eb7394
[ty] Preserve qualifiers when accessing attributes on unions/intersec…
sharkdp 178b902
[`airflow`] Extend `AIR311` and `AIR312` rules (#20082)
Lee-W eb59daf
[ty] Optimize TDD atom ordering (#20098)
sharkdp 8e9da9a
[`ruff`] Preserve relative whitespace in multi-line expressions (`RUF…
danparizher a75a589
[ty] print diagnostics with fully qualified name to disambiguate some…
leandrobbraga 7de87bb
[`flake8-async`] Implement `blocking-http-call-httpx` (`ASYNC212`) (#…
amyreese c740403
[ty] Introduce a representation for the top/bottom materialization of…
JelleZijlstra 8922255
[ty] Evaluate reachability of non-definitely-bound to Ambiguous (#19579)
Glyphack cb1db5e
Move GitLab output rendering to `ruff_db` (#20117)
ntBre d2ffe00
[`pyflakes`] Fix `allowed-unused-imports` matching for top-level modu…
TaKO8Ki 6f67b0e
[ty] Benchmarks for problematic implicit instance attributes cases (#…
sharkdp b439986
Bump 0.12.11 (#20136)
ntBre b0856d6
[ty] No boundness analysis for implicit instance attributes (#20128)
sharkdp eb32ba3
[ty] Fix 'too many cycle iterations' for unions of literals (#20137)
sharkdp 696784b
Use new diff rendering format in tests (#20101)
ntBre fe4114e
[ty] add cycle detection for find_legacy_typevars (#20124)
carljm d0b7055
[ty] add support for cyclic legacy generic protocols (#20125)
carljm e835b45
Fix mdtest ignore python code blocks (#20139)
MatthewMckee4 dafd6ef
[`flake8-async`] Implement `blocking-input` rule (`ASYNC250`) (#20122)
amyreese 2bcbc3e
[ty] Simplify materialization of specialized generics (#20121)
JelleZijlstra ba446a4
[`pycodestyle`] Preserve return type annotation for `ParamSpec` (`E73…
danparizher 5a94fd7
[`pylint`] Add U+061C to `PLE2502` (#20106)
kotx 859f968
[`ruff`] Fix false negative for empty f-strings in `deque` calls (`RU…
danparizher b2ec901
[`perflint`] Handle tuples in dictionary comprehensions (`PERF403`) (…
liortct 1ea3e2b
[ty] Add constraint set implementation (#19997)
dcreager cc101f6
[ty] Unpack variadic argument type in specialization (#20130)
dhruvmanila c6bbb33
[ty] Enforce that an attribute on a class `X` must be callable in ord…
AlexWaygood b44952b
[ty] Improve disambiguation of types via fully qualified names (#20141)
AlexWaygood 8438cc2
[ty] Better error message for attempting to assign to a read-only pro…
AlexWaygood e682390
[`fastapi`] Fix false positive for paths with spaces around parameter…
danparizher 08f3f22
[`refurb`] Add fix safety section (`FURB105`) (#17499)
Kalmaegi f272eb8
Show fixes by default (#19919)
ntBre e592973
[`pyupgrade`] Add fix safety section to docs (`UP029`) (#17490)
Kalmaegi 3bf43d9
[ty] typecheck dict methods for `TypedDict` (#19874)
PrettyWood 3db8edd
[ty] ensure union normalization really normalizes (#20147)
carljm 6768220
[ty] minor TypedDict fixes (#20146)
carljm 34a6862
[ty] Use `invalid-assignment` error code for invalid assignments to `…
AlexWaygood e2085a6
[ty] add six ecosystem projects to good.txt (#20157)
carljm b12bfda
Revert "[ty] Use `invalid-assignment` error code for invalid assignme…
AlexWaygood d2822e1
[ty] skip a slow seed in fuzzer (#20161)
carljm 83261a0
Less confidently mark f-strings as empty when inferring truthiness (#…
dylwil3 2c857c3
[ty] don't assume that deferred type inference means deferred name re…
carljm 1da9e47
[ty] improve cycle-detection coverage for apply_type_mapping (#20159)
carljm 5fdbc0c
[ty] Sync vendored typeshed stubs (#20188)
github-actions[bot] a0da738
Update Rust crate mimalloc to v0.1.48 (#20187)
renovate[bot] 9e4370f
Update Rust crate clap to v4.5.46 (#20186)
renovate[bot] df02b42
Update Rust crate camino to v1.1.12 (#20185)
renovate[bot] 06bf0b9
Update cargo-bins/cargo-binstall action to v1.15.3 (#20182)
renovate[bot] 3eee9d3
Update CodSpeedHQ/action action to v3.8.1 (#20183)
renovate[bot] eed6a1f
Update rui314/setup-mold digest to 725a879 (#20181)
renovate[bot] 135d464
Update dependency ruff to v0.12.11 (#20184)
renovate[bot] 60f149d
[ty] Support `__init_subclass__` (#20190)
sharkdp c85a1a1
[ty] `__class_getitem__` is a classmethod (#20192)
sharkdp aa5beb3
[ty] add more lsp tests for overloads (#20148)
Gankra 18ceec5
[`flake8-use-pathlib`] Make `PTH119` and `PTH120` fixes unsafe becaus…
chirizxc 1b6d00c
[ty] Make initializer calls GotoTargets (#20014)
Gankra 8f5d225
[ty] Update ecosystem-analyzer (#20209)
sharkdp 5ac5305
[ty] Run ecosystem-analyzer in release mode (#20210)
sharkdp a2ec8a9
[ty] Add GitLab output format (#20155)
ntBre 9def095
[`airflow`] Improve the `AIR002` error message (#20173)
Lee-W d000a3d
[ty] Require that we can find a root when listing sub-modules
BurntSushi 2091d18
[ty] Make `Module::all_submodules` return `Module` instead of `Name`
BurntSushi 8030713
[ty] Add base for "all symbols" implementation
BurntSushi 1c99d8a
[ty] Add naive implementation of completions for unimported symbols
BurntSushi 1497f09
[ty] Make auto-import completions opt-in via an experimental option
BurntSushi e4d4307
[ty] Pick up typed text as a query for unimported completions
BurntSushi 3250f2e
[ty] Correct default value for experimental rename setting
BurntSushi 2414829
[`airflow`] Convert `DatasetOrTimeSchedule(datasets=...)` to `AssetOr…
Lee-W de3714a
[syntax-errors] Detect `yield from` inside async function (#20051)
11happy 2fb4499
[`airflow`] Move `airflow.operators.postgres_operator.Mapping` from `…
Lee-W 40a1e4b
Update Rust crate tracing-subscriber to v0.3.20 (#20162)
renovate[bot] 5d048d4
[ty]eliminate definitely-impossible types from union in equality narr…
Renkai e26c701
Expose `Indentation` in `ruff_python_codegen` (#20216)
ShaharNaveh da56213
[ty] Treat `__new__` as a static method (#20212)
sharkdp f1298b2
[`flake8-comprehensions`] Skip `C417` when lambda contains `yield`/`y…
danparizher 969ce80
[ty] Add functions for revealing assignability/subtyping constraints …
dcreager c5db2a4
[ty] Fix small test typo (#20220)
s-rigaud 6db395d
Split LICENSE addendum by derivation type (#20222)
charliermarsh 98edce4
[ty] More tests for TypedDict (#20205)
sharkdp 4a8e2cd
Bump 0.12.12 (#20242)
dylwil3 e65f868
[ty] Minor cleanups (#20240)
AlexWaygood c6ebb93
[ty] Attribute access on top/bottom materializations (#20221)
JelleZijlstra 06f38c0
[`ruff`] Use helper function for empty f-string detection in `in-empt…
TaKO8Ki a078253
[ty] Narrow specialized generics using isinstance() (#20256)
JelleZijlstra c71e05d
[ty] Reduce false positives for `ParamSpec`s and `TypeVarTuple`s (#20…
AlexWaygood 3ea99e8
[ty] TypedDict: Add support for `typing.ReadOnly` (#20241)
sharkdp 1e3bbd8
[ty] Minor: 'can not' => cannot (#20260)
sharkdp 7437a77
[ty] Cover full range of annotated assignments (#20261)
sharkdp b0ac987
[ty] Add backreferences to TypedDict items in diagnostics (#20262)
sharkdp f56e08c
[ty] Include `python` folder in `environment.root` if it exists (#20263)
sharkdp b73ffac
[ty] Update mypy_primer, add egglog-python project (#20078)
sharkdp 29b0bae
[ty] add doc-comments for some variance stuff (#20189)
ericmarkmartin f583551
[ty] Implement the legacy PEP-484 convention for indicating positiona…
AlexWaygood 6061f3a
Add support for sorting diagnostics without a range (#20257)
amyreese 39b3b9f
[ty] propagate visitors and constraints through has_relation_in_invar…
carljm 577c180
[ty] initial support for `slots=True` in dataclasses (#20278)
thejchap 1be758b
Update Rust crate insta to v1.43.2 (#20296)
renovate[bot] 0c385a1
Update Rust crate log to v0.4.28 (#20297)
renovate[bot] 8d70991
Update Rust crate wasm-bindgen-test to v0.3.51 (#20298)
renovate[bot] 51fc1ea
Update Rust crate clap to v4.5.47 (#20295)
renovate[bot] 0bc2112
Update Rust crate bitflags to v2.9.4 (#20294)
renovate[bot] 7c926d1
Update astral-sh/setup-uv action to v6.6.1 (#20291)
renovate[bot] e2b4e25
Update dependency mdformat-mkdocs to v4.4.1 (#20299)
renovate[bot] 4318f79
Update cargo-bins/cargo-binstall action to v1.15.4 (#20292)
renovate[bot] 850f35c
Update dependency ruff to v0.12.12 (#20293)
renovate[bot] 464edf2
CI: Eliminate warning in fuzz build workflow (#20290)
ferdnyc 55d7c47
[ty] Fall back to `object` for attribute access on synthesized protoc…
AlexWaygood 0df3741
[ty] Fix signature of `NamedTupleLike._make` (#20302)
sharkdp 1211217
[`pep8-naming`] Fix formatting of `__all__` (`N816`) (#20301)
onerandomusername c76b9aa
[ty] Support "legacy" `typing.Self` in combination with PEP 695 gener…
sharkdp 3dcbad7
[ty] Add support for generic PEP695 type aliases (#20219)
ibraheemdev 897e05f
[ty] more precise lazy scope place lookup (#19932)
mtshiba 65f9f72
[ty] equality narrowing on enums that don't override `__eq__` or `__n…
Renkai 2ea22da
Allow the `if_not_else` Clippy lint
BurntSushi 87738ff
[`pyupgrade`] Enable rule triggering for stub files (`UP043`) (#20027)
IDrokin117 c4d0eb6
Add support for using uv as an alternative formatter backend (#19665)
zanieb 1af1457
[ruff] #19982 add isolation rule for PYI021
second-ed 1dc7435
[flake8-pyi] add new test for isolation check for rule (PYI021)
second-ed 2150937
[flake8-pyi] update snapshots for tests (PYI021)
second-ed 73e2ccd
Merge branch 'main' into fix/PYI021_isolation_level
second-ed 0ecb545
[flake8-pyi] fix comments from review (PYI021)
second-ed 3f51950
nits
ntBre File filter
Filter by extension
Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
There are no files selected for viewing
18 changes: 18 additions & 0 deletions
18
crates/ruff_linter/resources/test/fixtures/flake8_pyi/PYI021_1.pyi
This file contains hidden or bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
| Original file line number | Diff line number | Diff line change |
|---|---|---|
| @@ -0,0 +1,18 @@ | ||
| from contextlib import nullcontext | ||
|
|
||
|
|
||
| def check_isolation_level(mode: int) -> None: | ||
| """Will report both, but only fix the first.""" # ERROR PYI021 | ||
| ... # ERROR PIE790 | ||
|
|
||
|
|
||
| with nullcontext(): | ||
| """Should not report.""" | ||
| # add something thats not a pass here | ||
| # to not get a remove unnecessary pass err | ||
| x = 0 | ||
|
|
||
| if True: | ||
| """Should not report.""" | ||
| # same as above | ||
| y = 1 |
This file contains hidden or bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
This file contains hidden or bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
This file contains hidden or bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
This file contains hidden or bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
This file contains hidden or bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
39 changes: 39 additions & 0 deletions
39
...8_pyi/snapshots/ruff_linter__rules__flake8_pyi__tests__pyi021_pie790_isolation_check.snap
This file contains hidden or bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
| Original file line number | Diff line number | Diff line change |
|---|---|---|
| @@ -0,0 +1,39 @@ | ||
| --- | ||
| source: crates/ruff_linter/src/rules/flake8_pyi/mod.rs | ||
| --- | ||
| PYI021 [*] Docstrings should not be included in stubs | ||
| --> PYI021_1.pyi:5:5 | ||
| | | ||
| 4 | def check_isolation_level(mode: int) -> None: | ||
| 5 | """Will report both, but only fix the first.""" # ERROR PYI021 | ||
| | ^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^ | ||
| 6 | ... # ERROR PIE790 | ||
| | | ||
| help: Remove docstring | ||
| 2 | | ||
| 3 | | ||
| 4 | def check_isolation_level(mode: int) -> None: | ||
| - """Will report both, but only fix the first.""" # ERROR PYI021 | ||
| 5 + # ERROR PYI021 | ||
| 6 | ... # ERROR PIE790 | ||
| 7 | | ||
| 8 | | ||
| note: This is an unsafe fix and may change runtime behavior | ||
|
|
||
| PIE790 [*] Unnecessary `...` literal | ||
| --> PYI021_1.pyi:6:5 | ||
| | | ||
| 4 | def check_isolation_level(mode: int) -> None: | ||
| 5 | """Will report both, but only fix the first.""" # ERROR PYI021 | ||
| 6 | ... # ERROR PIE790 | ||
| | ^^^ | ||
| | | ||
| help: Remove unnecessary `...` | ||
| 3 | | ||
| 4 | def check_isolation_level(mode: int) -> None: | ||
| 5 | """Will report both, but only fix the first.""" # ERROR PYI021 | ||
| - ... # ERROR PIE790 | ||
| 6 + # ERROR PIE790 | ||
| 7 | | ||
| 8 | | ||
| 9 | with nullcontext(): |
Oops, something went wrong.
Add this suggestion to a batch that can be applied as a single commit.
This suggestion is invalid because no changes were made to the code.
Suggestions cannot be applied while the pull request is closed.
Suggestions cannot be applied while viewing a subset of changes.
Only one suggestion per line can be applied in a batch.
Add this suggestion to a batch that can be applied as a single commit.
Applying suggestions on deleted lines is not supported.
You must change the existing code in this line in order to create a valid suggestion.
Outdated suggestions cannot be applied.
This suggestion has been applied or marked resolved.
Suggestions cannot be applied from pending reviews.
Suggestions cannot be applied on multi-line comments.
Suggestions cannot be applied while the pull request is queued to merge.
Suggestion cannot be applied right now. Please check back later.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
One thing I didn't realize about moving this here is that it now gets called for non-function (or -class or -module) suites, such as context managers or
ifstatements:I tested these locally, and we flag both of these. Fortunately, it looks like we already track a
docstring_statefield onChecker. We'd have to add apub(crate)getter function for this and make theDocstringStateandExpectedDocstringKindenumspub(crate)too, but I think we can use that to check that we're in an actual docstring.There's an existing
SemanticModel::in_pep_257_docstringmethod, but unfortunately that flag only gets set on theSemanticModelonce we're actually visiting the docstring.There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Hey thanks for this, I think I've addressed all of the comments from the review, please let me know if I've missed anything!