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.

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

plugins/spectre: set freeformType for engine options #1952

Merged
merged 1 commit into from
Aug 5, 2024

Conversation

elohmeier
Copy link
Contributor

This adds the warn engine option to be able to silence messages when an executable is missing from the PATH. See nvim-pack/nvim-spectre#242 for the upstream change.

@MattSturgeon
Copy link
Member

Thanks for you contribution! However, due to settings being a "freeform" option this change shouldn't be needed in order to define the corresponding config.

If you're unable to define the config then that means we have a bigger issue such as a sub-option blocking access to the freeformType.

@elohmeier
Copy link
Contributor Author

I'm not sure if it is a freeform option. To reproduce, I've modified the plugin test like this:

diff --git a/tests/test-sources/plugins/utils/spectre.nix b/tests/test-sources/plugins/utils/spectre.nix
index 5cb545c2..8ee2ce8a 100644
--- a/tests/test-sources/plugins/utils/spectre.nix
+++ b/tests/test-sources/plugins/utils/spectre.nix
@@ -249,6 +249,7 @@ in
         replace_engine = {
           sed = {
             cmd = "sed";
+            warn = false;
             args = [
               "-i"
               "-E"

Running the test with nix build .#checks.x86_64-linux.test-plugins-utils-spectre fails:

error:
       … while calling the 'derivationStrict' builtin
         at <nix/derivation-internal.nix>:9:12:
            8|
            9|   strict = derivationStrict drvAttrs;
             |            ^
           10|

       … while evaluating derivation 'plugins-utils-spectre'
         whose name attribute is located at /nix/store/bcghcr9qwqmanpds017w75mcqda4fgab-source/pkgs/stdenv/generic/make-derivation.nix:334:7

       … while evaluating attribute 'buildPhase' of derivation 'plugins-utils-spectre'
         at /nix/store/8pyswsp647284abvi94y83csbxidrydb-source/tests/test-derivation.nix:38:7:
           37|       # errors on stderr
           38|       buildPhase = lib.optionalString (!dontRun) (
             |       ^
           39|         ''

       (stack trace truncated; use '--show-trace' to show the full trace)

       error: The option `plugins.spectre.settings.replace_engine.sed.warn' does not exist. Definition values:
       - In `<unknown-file>': false

@elohmeier
Copy link
Contributor Author

I've updated this PR to set the missing freeformType option instead. The above test now works.

@elohmeier elohmeier changed the title plugins/utils/spectre: add warn engine option plugins/utils/spectre: set freeformType for engine options Aug 1, 2024
Copy link
Member

@MattSturgeon MattSturgeon left a comment

Choose a reason for hiding this comment

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

Much better! Small discussion point that may or may not need changing, then I think we're good to merge.

Also: minor nit on the commit message, we don't usually include the full verbose path plugins/utils/spector:; usually `plugins/spector: would be fine 😀

plugins/utils/spectre.nix Outdated Show resolved Hide resolved
@elohmeier elohmeier changed the title plugins/utils/spectre: set freeformType for engine options plugins/spectre: set freeformType for engine options Aug 2, 2024
@MattSturgeon
Copy link
Member

There's a few tests failing (even after I tried a rebuild).

Struggling to read the logs on my phone though, so you may have to investigate on your own for now.

Copy link
Member

@MattSturgeon MattSturgeon left a comment

Choose a reason for hiding this comment

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

Seems tests all good now!

Did you want to rebase and get full commit attribution?

@GaetanLepage
Copy link
Member

@Mergifyio queue

Copy link
Contributor

mergify bot commented Aug 4, 2024

queue

🛑 The pull request has been removed from the queue default

The queue conditions cannot be satisfied due to failing checks.

You can take a look at Queue: Embarked in merge queue check runs for more details.

In case of a failure due to a flaky test, you should first retrigger the CI.
Then, re-embark the pull request into the merge queue by posting the comment
@mergifyio refresh on the pull request.

@elohmeier
Copy link
Contributor Author

Great, thanks. I don't want to rebase - let's get it in!

@GaetanLepage
Copy link
Member

@Mergifyio refresh

Copy link
Contributor

mergify bot commented Aug 5, 2024

refresh

✅ Pull request refreshed

@GaetanLepage
Copy link
Member

@Mergifyio queue

Copy link
Contributor

mergify bot commented Aug 5, 2024

queue

🛑 The pull request has been removed from the queue default

The queue conditions cannot be satisfied due to failing checks.

You can take a look at Queue: Embarked in merge queue check runs for more details.

In case of a failure due to a flaky test, you should first retrigger the CI.
Then, re-embark the pull request into the merge queue by posting the comment
@mergifyio refresh on the pull request.

@GaetanLepage
Copy link
Member

@Mergifyio refresh

Copy link
Contributor

mergify bot commented Aug 5, 2024

refresh

✅ Pull request refreshed

@GaetanLepage
Copy link
Member

https://github.com/Mergifyio queue

Copy link
Contributor

mergify bot commented Aug 5, 2024

queue

✅ The pull request has been merged automatically

The pull request has been merged automatically at 4d6d3bd

@mergify mergify bot merged commit 4d6d3bd into nix-community:main Aug 5, 2024
4 checks passed
@mergify mergify bot temporarily deployed to github-pages August 5, 2024 07:59 Inactive
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

4 participants