358: Add support for formatting entire document with rustfmt r=matklad a=aleksanb
Attempting to format a document when rustfmt isn't installed will result
in an error being returned to the frontend. An alternative
implementation would be returning zero replacements.
Part of https://github.com/rust-analyzer/rust-analyzer/issues/160.
Co-authored-by: Aleksander Vognild Burkow <aleksanderburkow@gmail.com>
Attempting to format a document when rustfmt isn't installed will result
in an error being returned to the frontend. An alternative
implementation would be returning zero replacements.
356: Fix a bug in char literal validation discovered through fuzzing r=matklad a=DJMcNab
We also add a Cargo.lock to the fuzzing directory, as that isn't gitignored automatically, so I imagine it should be committed.
Co-authored-by: DJMcNab <36049421+djmcnab@users.noreply.github.com>
This will really become necessary when we implement generics, but even now, it
allows us to reason 'backwards' to infer types of expressions that we didn't
understand for some reason.
We use ena, the union-find implementation extracted from rustc, to keep track of
type variables.
350: Super simple macro support r=matklad a=matklad
Super simple support for macros, mostly for figuring out how to fit them into the current architecture. Expansion is hard-coded and string based (mid-term, we should try to copy-paste macro-by-example expander from rustc).
Ideally, we should handle
* highlighting inside the macro (done)
* extend selection inside the macro
* completion inside the macro
* indexing structs, produced by the macro
Co-authored-by: Aleksey Kladov <aleksey.kladov@gmail.com>
341: Bump languageserver-types from 0.53.0 to 0.53.1 r=matklad a=dependabot[bot]
Bumps [languageserver-types](https://github.com/gluon-lang/languageserver-types) from 0.53.0 to 0.53.1.
<details>
<summary>Commits</summary>
- [`1a6c6c1`](1a6c6c18fc) (cargo-release) version 0.53.1
- [`5591192`](5591192047) Merge pull request [#88](https://github-redirect.dependabot.com/gluon-lang/languageserver-types/issues/88) from Xanewok/hover-clone
- [`c78120b`](c78120bf28) Add `Clone` impl for HoverContents and related
- See full diff in [compare view](https://github.com/gluon-lang/languageserver-types/compare/v0.53.0...v0.53.1)
</details>
<br />
[![Dependabot compatibility score](https://api.dependabot.com/badges/compatibility_score?dependency-name=languageserver-types&package-manager=cargo&previous-version=0.53.0&new-version=0.53.1)](https://dependabot.com/compatibility-score.html?dependency-name=languageserver-types&package-manager=cargo&previous-version=0.53.0&new-version=0.53.1)
Dependabot will resolve any conflicts with this PR as long as you don't alter it yourself. You can also trigger a rebase manually by commenting `@dependabot rebase`.
[//]: # (dependabot-automerge-start)
[//]: # (dependabot-automerge-end)
---
**Note:** This repo was added to Dependabot recently, so you'll receive a maximum of 5 PRs for your first few update runs. Once an update run creates fewer than 5 PRs we'll remove that limit.
You can always request more updates by clicking `Bump now` in your [Dependabot dashboard](https://app.dependabot.com).
<details>
<summary>Dependabot commands and options</summary>
<br />
You can trigger Dependabot actions by commenting on this PR:
- `@dependabot rebase` will rebase this PR
- `@dependabot recreate` will recreate this PR, overwriting any edits that have been made to it
- `@dependabot merge` will merge this PR after your CI passes on it
- `@dependabot cancel merge` will cancel a previously requested merge
- `@dependabot reopen` will reopen this PR if it is closed
- `@dependabot ignore this [patch|minor|major] version` will close this PR and stop Dependabot creating any more for this minor/major version (unless you reopen the PR or upgrade to it yourself)
- `@dependabot ignore this dependency` will close this PR and stop Dependabot creating any more for this dependency (unless you reopen the PR or upgrade to it yourself)
- `@dependabot use these labels` will set the current labels as the default for future PRs for this repo and language
- `@dependabot use these reviewers` will set the current reviewers as the default for future PRs for this repo and language
- `@dependabot use these assignees` will set the current assignees as the default for future PRs for this repo and language
- `@dependabot use this milestone` will set the current milestone as the default for future PRs for this repo and language
- `@dependabot badge me` will comment on this PR with code to add a "Dependabot enabled" badge to your readme
Additionally, you can set the following in your Dependabot [dashboard](https://app.dependabot.com):
- Update frequency (including time of day and day of week)
- Automerge options (never/patch/minor, and dev/runtime dependencies)
- Pull request limits (per update run and/or open at any time)
- Out-of-range updates (receive only lockfile updates, if desired)
- Security updates (receive only security updates, if desired)
Finally, you can contact us by mentioning @dependabot.
</details>
Co-authored-by: dependabot[bot] <support@dependabot.com>
343: Bump arrayvec from 0.4.9 to 0.4.10 r=matklad a=dependabot[bot]
Bumps [arrayvec](https://github.com/bluss/arrayvec) from 0.4.9 to 0.4.10.
<details>
<summary>Commits</summary>
- [`21661fa`](21661facf8) 0.4.10
- [`06930d2`](06930d27ce) FIX: Remove unused Copy/Clone for MaybeUninit
- [`85d9a06`](85d9a06a62) FIX: Use repr(C) MaybeUninit after discussion with RalfJung
- See full diff in [compare view](https://github.com/bluss/arrayvec/compare/0.4.9...0.4.10)
</details>
<br />
[![Dependabot compatibility score](https://api.dependabot.com/badges/compatibility_score?dependency-name=arrayvec&package-manager=cargo&previous-version=0.4.9&new-version=0.4.10)](https://dependabot.com/compatibility-score.html?dependency-name=arrayvec&package-manager=cargo&previous-version=0.4.9&new-version=0.4.10)
Dependabot will resolve any conflicts with this PR as long as you don't alter it yourself. You can also trigger a rebase manually by commenting `@dependabot rebase`.
[//]: # (dependabot-automerge-start)
[//]: # (dependabot-automerge-end)
---
**Note:** This repo was added to Dependabot recently, so you'll receive a maximum of 5 PRs for your first few update runs. Once an update run creates fewer than 5 PRs we'll remove that limit.
You can always request more updates by clicking `Bump now` in your [Dependabot dashboard](https://app.dependabot.com).
<details>
<summary>Dependabot commands and options</summary>
<br />
You can trigger Dependabot actions by commenting on this PR:
- `@dependabot rebase` will rebase this PR
- `@dependabot recreate` will recreate this PR, overwriting any edits that have been made to it
- `@dependabot merge` will merge this PR after your CI passes on it
- `@dependabot cancel merge` will cancel a previously requested merge
- `@dependabot reopen` will reopen this PR if it is closed
- `@dependabot ignore this [patch|minor|major] version` will close this PR and stop Dependabot creating any more for this minor/major version (unless you reopen the PR or upgrade to it yourself)
- `@dependabot ignore this dependency` will close this PR and stop Dependabot creating any more for this dependency (unless you reopen the PR or upgrade to it yourself)
- `@dependabot use these labels` will set the current labels as the default for future PRs for this repo and language
- `@dependabot use these reviewers` will set the current reviewers as the default for future PRs for this repo and language
- `@dependabot use these assignees` will set the current assignees as the default for future PRs for this repo and language
- `@dependabot use this milestone` will set the current milestone as the default for future PRs for this repo and language
- `@dependabot badge me` will comment on this PR with code to add a "Dependabot enabled" badge to your readme
Additionally, you can set the following in your Dependabot [dashboard](https://app.dependabot.com):
- Update frequency (including time of day and day of week)
- Automerge options (never/patch/minor, and dev/runtime dependencies)
- Pull request limits (per update run and/or open at any time)
- Out-of-range updates (receive only lockfile updates, if desired)
- Security updates (receive only security updates, if desired)
Finally, you can contact us by mentioning @dependabot.
</details>
344: Bump itertools from 0.7.11 to 0.8.0 r=matklad a=dependabot[bot]
Bumps [itertools](https://github.com/bluss/rust-itertools) from 0.7.11 to 0.8.0.
<details>
<summary>Commits</summary>
- [`cd0602a`](cd0602addc) 0.8.0
- [`5a8f2fd`](5a8f2fd5ed) MAINT: Require Rust 1.24 as minimum version
- [`4986d92`](4986d92d7f) DOC: Minor edits to module docs
- [`01f15a0`](01f15a0910) Merge [#288](https://github-redirect.dependabot.com/bluss/rust-itertools/issues/288)
- [`883d40a`](883d40a6ef) map_into method
- [`3bf265d`](3bf265d5b7) Merge pull request [#321](https://github-redirect.dependabot.com/bluss/rust-itertools/issues/321) from JohnHeitmann/master
- [`e820996`](e820996f64) Document the trait extension behavior of Itertools a bit more clearly
- [`44c9654`](44c9654fdb) Merge pull request [#318](https://github-redirect.dependabot.com/bluss/rust-itertools/issues/318) from bluss/std-deprecations
- [`d2e254f`](d2e254f22f) API: Fix the mystery deprecation message for Step
- [`602f2f6`](602f2f675e) API: Deprecate .foreach() in favour of std's .for_each()
- Additional commits viewable in [compare view](https://github.com/bluss/rust-itertools/compare/0.7.11...0.8.0)
</details>
<br />
[![Dependabot compatibility score](https://api.dependabot.com/badges/compatibility_score?dependency-name=itertools&package-manager=cargo&previous-version=0.7.11&new-version=0.8.0)](https://dependabot.com/compatibility-score.html?dependency-name=itertools&package-manager=cargo&previous-version=0.7.11&new-version=0.8.0)
Dependabot will resolve any conflicts with this PR as long as you don't alter it yourself. You can also trigger a rebase manually by commenting `@dependabot rebase`.
[//]: # (dependabot-automerge-start)
[//]: # (dependabot-automerge-end)
---
**Note:** This repo was added to Dependabot recently, so you'll receive a maximum of 5 PRs for your first few update runs. Once an update run creates fewer than 5 PRs we'll remove that limit.
You can always request more updates by clicking `Bump now` in your [Dependabot dashboard](https://app.dependabot.com).
<details>
<summary>Dependabot commands and options</summary>
<br />
You can trigger Dependabot actions by commenting on this PR:
- `@dependabot rebase` will rebase this PR
- `@dependabot recreate` will recreate this PR, overwriting any edits that have been made to it
- `@dependabot merge` will merge this PR after your CI passes on it
- `@dependabot cancel merge` will cancel a previously requested merge
- `@dependabot reopen` will reopen this PR if it is closed
- `@dependabot ignore this [patch|minor|major] version` will close this PR and stop Dependabot creating any more for this minor/major version (unless you reopen the PR or upgrade to it yourself)
- `@dependabot ignore this dependency` will close this PR and stop Dependabot creating any more for this dependency (unless you reopen the PR or upgrade to it yourself)
- `@dependabot use these labels` will set the current labels as the default for future PRs for this repo and language
- `@dependabot use these reviewers` will set the current reviewers as the default for future PRs for this repo and language
- `@dependabot use these assignees` will set the current assignees as the default for future PRs for this repo and language
- `@dependabot use this milestone` will set the current milestone as the default for future PRs for this repo and language
- `@dependabot badge me` will comment on this PR with code to add a "Dependabot enabled" badge to your readme
Additionally, you can set the following in your Dependabot [dashboard](https://app.dependabot.com):
- Update frequency (including time of day and day of week)
- Automerge options (never/patch/minor, and dev/runtime dependencies)
- Pull request limits (per update run and/or open at any time)
- Out-of-range updates (receive only lockfile updates, if desired)
- Security updates (receive only security updates, if desired)
Finally, you can contact us by mentioning @dependabot.
</details>
Co-authored-by: dependabot[bot] <support@dependabot.com>
325: implement translate_offset_with_edit r=matklad a=vemoo
- Implement `translate_offset_with_edit` to resolve#105
- Add proptest impls for text, offsets and edits and use them in tests for `translate_offset_with_edit` and `LineIndex`
- Added benchmark for `translate_offset_with_edit`
Co-authored-by: Bernardo <berublan@gmail.com>
332: Struct types r=matklad a=flodiebold
Infer types for struct fields, and add basic field completions. There's also some code for enums, but I focused on getting structs working.
There's still ways to go before this becomes useful: There's no autoderef (or even reference types) and no inference for `self`, for example.
Co-authored-by: Florian Diebold <flodiebold@gmail.com>
We need to be able to get the receiver even if there is no field name yet, and
currently "a." wouldn't get parsed as a field name at all. This seems to help.
326: resolved#324: remove unnecessary braces in use statement. r=matklad a=gfreezy
Add inspection for unnecessary braces in use statement
Co-authored-by: gfreezy <gfreezy@gmail.com>
327: Beginnings of type inference r=flodiebold a=flodiebold
I was a bit bored, so I thought I'd try to start implementing the type system and see how far I come 😉 This is obviously still extremely WIP, only very basic stuff working, but I thought I'd post this now to get some feedback as to whether this approach makes sense at all.
There's no user-visible effect yet, but the type inference has tests similar to the ones for the parser. My next step will probably be to implement struct types, after which this could probably be used to complete fields.
I realize this may all get thrown away when/if the compiler query system gets usable, but I feel like there are lots of IDE features that could be implemented with somewhat working type inference in the meantime 😄
Co-authored-by: Florian Diebold <flodiebold@gmail.com>
302: WIP: Support tracing lsp requests. r=DJMcNab a=DJMcNab
EDIT: We need to work out a better way to handle settings before this can be merged. Help wanted
TODO: Debug why decorations are sent even when highlightingOn is disabled
This makes the log volume so high its impossible to work with anyway.
(Continuation of #84 [#99 only disabled using it, not making sure we don't send it]).
These logs can be used in https://microsoft.github.io/language-server-protocol/inspector/
Co-authored-by: DJMcNab <36049421+djmcnab@users.noreply.github.com>
This improving this code is not a good use of people-time, and this
might be the most performant approach nonwithstanding
an api for this use case being added to walkdir
315: Split completion into manageable components r=matklad a=matklad
The main idea here is to do completion in two phases:
* first, we figure out surrounding context
* then, we run a series of completers on the given context.
Co-authored-by: Aleksey Kladov <aleksey.kladov@gmail.com>
306: Finish weird exprs r=DJMcNab a=DJMcNab
Fix#290.
Note that I'm not certain my use of `p.nth(1) == Ident` is entirely consistent with `libsyntax` - in the original, [`is_union_item`](9622f9dc47/src/libsyntax/parse/parser.rs (L4593-L4596)) uses `t.is_ident() && !t.is_reserved_ident()`, whereas we effectively only do `is_ident`. However, I cannot find the definition of `is_reserved_ident` (even searching the rust repository only gives uses, no definitions), so this will have to do unless someone else can find it :|.
Co-authored-by: DJMcNab <36049421+djmcnab@users.noreply.github.com>
This isn't working properly because we don't dynamically disable or enable it
TODO: work out why highlighting can be enabled mid session.
TODO: Improve settings handling
273: Add a test to ensure that we can parse each file r=matklad a=DJMcNab
Note that this has a non-spurious failure in ra_analysis/src/mock_analysis.
Probably fixes#195.
If my understanding is correct, fixes#214 and fixes#225.
Co-authored-by: DJMcNab <36049421+djmcnab@users.noreply.github.com>
This fixes a common problem when running under VS Code, the user
doesn't have permissions to create a `log` directory in the CWD.
The old behavior can be re-enabled by setting RA_INTERNAL_MODE=1
The cast expression expected any type via types::type_() function,
but the language spec does only allow TypeNoBounds (types without direct extra bounds
via `+`).
**Example:**
```rust
fn test() {
6i8 as i32 + 5;
}
```
This fails, because the types::type_() function which should parse the type after the
as keyword is greedy, and takes all plus sign after path types as extra.
My proposed fix is to replace the not implemented `type_no_plus()` just calls (`type_()`)
function, which is used at several places. The replacement is `type_with_bounds_cond(p: &mut Parser, allow_bounds: bool)`, which passes the condition to relevant sub-parsers.
This function is then called by `type_()` and the new public `type_no_bounds()`.
271: Implement format hook r=matklad a=DJMcNab
Tentatively: fixes#155.
However, this does add all changes in staged files, which might not be desirable. However, I think we can't solve that without explicit support in rustfmt for it, so it should be fine.
Co-authored-by: DJMcNab <36049421+djmcnab@users.noreply.github.com>
267: Fix the extend keybinding r=DJMcNab a=DJMcNab
Make the extend selection keybinding less annoying for users not used to Injelli-J (myself included). Also fixes a minor style issue and runs `npm update`.
Co-authored-by: DJMcNab <36049421+djmcnab@users.noreply.github.com>
265: Refactor symbol resolve API r=matklad a=matklad
Introduce ReferenceResolution to avoid nesting to many non-nominal
types.
Co-authored-by: Aleksey Kladov <aleksey.kladov@gmail.com>
256: Improve/add use_item documentation r=matklad a=DJMcNab
Adds some documentation to use_item explaining all code paths (use imports are hard, especially with the ongoing discussion of anchored v. uniform paths - see https://github.com/rust-lang/rust/issues/55618 for what appears to be the latest developments)
Co-authored-by: DJMcNab <36049421+djmcnab@users.noreply.github.com>
253: Fix diagnostic fixes showing up everywhere r=matklad a=flodiebold
The LSP code action request always returned the fixes for all diagnostics anywhere in the file, because of a shadowed variable.
There's no test yet; I wasn't sure where to add it. I tried adding one in `heavy_tests`, but that's a bit uncomfortable because the code action response contains the (random) file paths. I could make it work, but wanted to ask beforehand what you think.
Co-authored-by: Florian Diebold <flodiebold@gmail.com>
250: Improve the suggestion for test functions r=DJMcNab a=DJMcNab
I haven't fully updated the previous commented out test - I don't know why it was commented out so some clarification would be welcome.
Co-authored-by: Daniel McNab <36049421+djmcnab@users.noreply.github.com>
Co-authored-by: DJMcNab <36049421+djmcnab@users.noreply.github.com>
252: Improve 'introduce variable' r=matklad a=flodiebold
- make it possible to extract a prefix of an expression statement (e.g.
`<|>foo.bar()<|>.baz()`)
- don't turn the last expression in a block into a let statement
- also fix a few typos
Co-authored-by: Florian Diebold <flodiebold@gmail.com>
- make it possible to extract a prefix of an expression statement (e.g.
<|>foo.bar()<|>.baz())
- don't turn the last expression in a block into a let statement
"volatile" means "changes every time". That is, all transitive
rev-deps of volatile queries will be executed every time. We actually
need "dependencies".
Note that VSCode asks for completion after *first* `:` as well:
use crate:
we use hacks to protect against that, and to give completions only
after the second `:`.
207: Finish implementing char validation r=aochagavia a=aochagavia
The only thing missing right now are good integration tests (and maybe more descriptive error messages)
Co-authored-by: Adolfo Ochagavía <github@adolfo.ochagavia.xyz>
167: Attempt to extract useful comments from function signatures r=matklad a=kjeremy
I'm trying to extract useful function comments for signature info. This will also be useful for hover. This is a WIP (and actually works pretty well!) but I don't think it's the right approach long term so some guidance would be appreciated so that we could also get comments for say types and variable instances etc.
Currently `test_fn_signature_with_simple_doc` fails due to a bug in `extend` but we probably shouldn't use this approach anyway. Maybe comments should be attached to nodes somehow? I'm also thinking that maybe the markdown bits should live in the language server.
Thoughts?
Co-authored-by: Jeremy A. Kolb <jkolb@ara.com>
176: Move completio to ra_analysis r=matklad a=matklad
While we should handle completion for isolated file, it's better
achieved by using empty Analysis, rather than working only with &File:
we need memoization for type inference even inside a single file.
Co-authored-by: Aleksey Kladov <aleksey.kladov@gmail.com>
While we should handle completion for isolated file, it's better
achieved by using empty Analysis, rather than working only with &File:
we need memoization for type inference even inside a single file.
This is a first step towards queryifing completion and resolve.
Some code currently duplicates ra_editor: the plan is to move all
completion from ra_editor, but it'll take more than one commit.
157: Introduce ModuleId r=matklad a=matklad
Previously, module was synonym with a file, and so a module could have
had several parents. This commit introduces a separate module concept,
such that each module has only one parent, but a single file can
correspond to different modules.
Co-authored-by: Aleksey Kladov <aleksey.kladov@gmail.com>
Previously, module was synonym with a file, and so a module could have
had several parents. This commit introduces a separate module concept,
such that each module has only one parent, but a single file can
correspond to different modules.
156: Cargo Update run r=kjeremy a=kjeremy
Bump relative-path to 0.4.0
Failure 0.1.3 to fix leak with downcast
Updated everything else too
Co-authored-by: Jeremy A. Kolb <jkolb@ara.com>
138: Fix some clippy lints r=matklad a=alanhdu
I went ahead and fixed all the clippy lints (there were a couple I thought would be better unfixed and added `cfg` statements to allow them) and also re-enabled clippy and rustfmt in CI.
They were disabled with `no time to explain, disable clippy checks`, so hopefully this won't go against whatever the reason at the time was 😆.
One question about the CI though: right now, it's an allowed failure that runs against the latest nightly each time. Would it be better to pin it to a specific nightly (or use the `beta` versions) to lower the churn?
Co-authored-by: Alan Du <alanhdu@gmail.com>
143: Implement Find All References and Rename for local variables r=matklad a=kjeremy
Expose `find_all_refs` in `Analysis`. This currently only works for local variables.
Use this in the LSP to implement find all references and rename.
Co-authored-by: Jeremy A. Kolb <jkolb@ara.com>
128: Add a test to verify if the generated codes are up-to-date. r=matklad a=mominul
This test checks if the generated codes are up-to-date every time during `cargo test`.
I have confirmed that the test works by manually editing the `grammar.ron` file.
Closes#126
Thanks!
Co-authored-by: Muhammad Mominul Huque <mominul2082@gmail.com>
127: Improve folding r=matklad a=aochagavia
I was messing around with adding support for multiline comments in folding and ended up changing a bunch of other things.
First of all, I am not convinced of folding groups of successive items. For instance, I don't see why it is worthwhile to be able to fold something like the following:
```rust
use foo;
use bar;
```
Furthermore, this causes problems if you want to fold a multiline import:
```rust
use foo::{
quux
};
use bar;
```
The problem is that now there are two possible folds at the same position: we could fold the first use or we could fold the import group. IMO, the only place where folding groups makes sense is when folding comments. Therefore I have **removed folding import groups in favor of folding multiline imports**.
Regarding folding comments, I made it a bit more robust by requiring that comments can only be folded if they have the same flavor. So if you have a bunch of `//` comments followed by `//!` comments, you will get two separate fold groups instead of a single one.
Finally, I rewrote the API in such a way that it should be trivial to add new folds. You only need to:
* Create a new FoldKind
* Add it to the `fold_kind` function that converts from `SyntaxKind` to `FoldKind`
Fixes#113
Co-authored-by: Adolfo Ochagavía <github@adolfo.ochagavia.xyz>
Implements a pretty barebones function signature help mechanism in
the language server.
Users can use `Analysis::resolve_callback()` to get basic information
about a call site.
Fixes#102
116: Collapse comments upon join r=matklad a=aochagavia
Todo:
- [x] Write tests
- [x] Resolve fixmes
- [x] Implement `comment_start_length` using the parser
I left a bunch of questions as fixmes. Can someone take a look at them? Also, I would love to use the parser to calculate the length of the leading characters in a comment (`//`, `///`, `//!`, `/*`), so any hints are greatly appreciated.
Co-authored-by: Adolfo Ochagavía <aochagavia92@gmail.com>
Co-authored-by: Adolfo Ochagavía <github@adolfo.ochagavia.xyz>
118: Remove error publishing through publishDecorations r=matklad a=aochagavia
The errors are already reported by `publishDiagnostics`
Closes#109
Co-authored-by: Adolfo Ochagavía <aochagavia92@gmail.com>
98: WIP: Add resolve_local_name to resolve names in a function scope r=kjeremy a=kjeremy
First step to resolving #80
Co-authored-by: Jeremy A. Kolb <jkolb@ara.com>
93: Support leading pipe in match arms r=matklad a=DJMcNab
This adds support for match arms of the form:
```rust
<...>
| X | Y => <...>,
| X => <...>,
| 1..2 => <...>,
etc
```
# Implementation discussion
This just naïvely 'eats' a leading pipe if one is available. The equivalent line in the reference `libsyntax` is in [`parse_arm`](441519536c/src/libsyntax/parse/parser.rs (L3552)).
As noted in the comment linked above, this feature was formally introduced as a result of rust-lang/rfcs#1925. This feature is in active use in the [`rust-analyzer` codebase](c87fcb4ea5/crates/ra_syntax/src/syntax_kinds/generated.rs (L231))
I have added some tests for this feature, but maybe more would be required
EDIT: Always looking for feedback - is this PR description over-engineered?
Co-authored-by: Daniel McNab <36049421+djmcnab@users.noreply.github.com>