operators dell-csi-operator (1.7.0)

Thanks submitting your Operator. Please check below list before you create your Pull Request.
New Submissions

[ ] Are you familiar with our contribution guidelines?
[ ] Have you packaged and deployed your Operator for Operator Framework?

[ ] Have you tested your Operator in all supported installation modes?
[ ] Have you considered whether you want use semantic versioning order?
[ ] Is your submission signed?
[ ] Is operator icon set?

Updates to existing Operators

[x] Did you create a ci.yaml file according to the update instructions?
[x] Is your new CSV pointing to the previous version with the replaces property if you chose replaces-mode via the updateGraph property in ci.yaml?
[x] Is your new CSV referenced in the appropriate channel defined in the package.yaml or annotations.yaml ?
[x] Have you tested an update to your Operator when deployed via OLM?
[x] Is your submission signed?

Your submission should not

[x] Modify more than one operator
[x] Modify an Operator you don't own
[x] Rename an operator - please remove and add with a different name instead
[x] Modify any files outside the above mentioned folders
[x] Contain more than one commit. Please squash your commits.

Operator Description must contain (in order)

[x] Description about the managed Application and where to find more information
[x] Features and capabilities of your Operator and how to use it
[x] Any manual steps about potential pre-requisites for using your Operator

Operator Metadata should contain

[x] Human readable name and 1-liner description about your Operator
[x] Valid category name1
[x] One of the pre-defined capability levels2
[x] Links to the maintainer, source code and documentation
[x] Example templates for all Custom Resource Definitions intended to be used
[x] A quadratic logo

Remember that you can preview your CSV here.
--
1 If you feel your Operator does not fit any of the pre-defined categories, file an issue against this repo and explain your need
2 For more information see here

/retest

/retest-required

/retest

/test all

/test 4.7-deploy-operator-on-openshift

/test 4.6-deploy-operator-on-openshift

Hi @mvalarh , could you please help tell what is making the tests fail on openshift 4.6 and 4.7. We are not supporting these versions in this release. Is there something missing from our end

/test 4.6-deploy-operator-on-openshift
/test 4.7-deploy-operator-on-openshift

/test 4.7-deploy-operator-on-openshift

/test 4.6-deploy-operator-on-openshift

/merge possible

@rensyct operator is failing installation on 4.7, but is successful on 4.10. How is your local test on 4.7?

/test 4.6-deploy-operator-on-openshift

Hi @J0zi, support matrix for dell-csi-operator v1.7.0 is OCP 4.8 and OCP 4.9. So we have not tried an installation on OCP 4.7. Please let know how we can proceed with this. Is there some fields to be added in CSV which can tell that operator should be tested only on OCP 4.8 and OCP 4.9

we should not test on unsupported version, coming soon


Component #:BORDEAUX-THREADS not found, required by #<SYSTEM "sbcl-librarian">

After the commit https://github.com/quil-lang/sbcl-librarian/commit/04f7e390c777084ac43b8df9e90593a041da2381 attempt to build a libcalc example fails with this trace:
CL_SOURCE_REGISTRY="/home/art/projects/sbcl-librarian//" /home/art/.roswell/src/sbcl-2.3.2//run-sbcl.sh --script "script.lisp"
While evaluating the form starting at line 1, column 0
  of #P"/home/art/projects/sbcl-librarian/examples/libcalc/libcalc.asd":
Unhandled MISSING-DEPENDENCY in thread #<SB-THREAD:THREAD "main thread" RUNNING {1004490283}>: Component #:BORDEAUX-THREADS not found, required by #<SYSTEM "sbcl-librarian">

Backtrace for: #<SB-THREAD:THREAD "main thread" RUNNING {1004490283}>
0: (SB-DEBUG::DEBUGGER-DISABLED-HOOK Component #:BORDEAUX-THREADS not found, required by #<SYSTEM "sbcl-librarian"> #<unused argument> :QUIT T)
1: (SB-DEBUG::RUN-HOOK SB-EXT:*INVOKE-DEBUGGER-HOOK* Component #:BORDEAUX-THREADS not found, required by #<SYSTEM "sbcl-librarian">)
2: (INVOKE-DEBUGGER Component #:BORDEAUX-THREADS not found, required by #<SYSTEM "sbcl-librarian">)
3: (ERROR MISSING-DEPENDENCY :REQUIRED-BY #<SYSTEM "sbcl-librarian"> :REQUIRES #:BORDEAUX-THREADS)
4: (ASDF/FIND-COMPONENT:RESOLVE-DEPENDENCY-NAME #<SYSTEM "sbcl-librarian"> #:BORDEAUX-THREADS NIL)
5: (ASDF/PLAN:MAP-DIRECT-DEPENDENCIES #<PREPARE-OP > #<SYSTEM "sbcl-librarian"> #<FUNCTION (LAMBDA (ASDF/PLAN::O ASDF/PLAN::C) :IN ASDF/PLAN:TRAVERSE-ACTION) {100129BE9B}>)
6: ((LAMBDA NIL :IN ASDF/PLAN:TRAVERSE-ACTION))
7: ((LAMBDA NIL :IN ASDF/ACTION:CALL-WHILE-VISITING-ACTION))
8: (ASDF/PLAN:TRAVERSE-ACTION #<SEQUENTIAL-PLAN {1001276833}> #<PREPARE-OP > #<SYSTEM "sbcl-librarian"> T)
9: (ASDF/PLAN:MAP-DIRECT-DEPENDENCIES #<PREPARE-OP > #<CL-SOURCE-FILE "sbcl-librarian" "package"> #<FUNCTION (LAMBDA (ASDF/PLAN::O ASDF/PLAN::C) :IN ASDF/PLAN:TRAVERSE-ACTION) {100129BCEB}>)
10: ((LAMBDA NIL :IN ASDF/PLAN:TRAVERSE-ACTION))
11: ((LAMBDA NIL :IN ASDF/ACTION:CALL-WHILE-VISITING-ACTION))

Possible solution
Is to add https://github.com/sionescu/bordeaux-threads as submodule and to update Makefile to ensure that submodules are in sync. Actually not only bordeaux-threads should be cloned. Here is the full list of commands I did to solve the problem:
cd ~/projects/sbcl-librarian/

mkdir bundle
cd bundle

git clone https://github.com/sionescu/bordeaux-threads
git clone https://gitlab.common-lisp.net/alexandria/alexandria
git clone https://github.com/lmj/global-vars
git clone https://github.com/trivial-features/trivial-features
git clone https://github.com/trivial-garbage/trivial-garbage

Probably just using SB-thread directly would be better.


add an example for using the net/http adapter

Is there a way to turn this into a unit test (or add a unit test)?

I do have a unit test for the adapter in https://github.com/fastly/compute-sdk-go/blob/main/fsthttp/adapter_test.go.  I could create an integration test which uses this, but as it's entirely an SDK thing (no runtime interaction) I'm not sure it buys us anything.


Initialize color

Hi, thanks for this great color-picker.
Is there a ay to initialise it with a specific color?

How can you change the colours, and also can be made to work horizontally?


New Rule: Request for Quote or Purchase (RFQ|RFP) with HTML smuggling attachment

/update-test-rules

testing looks very solid on this, good overlap with rules, and some new pickups as well! nice @aidenmitchell


Grid Item cell panel location

the location of the Item cell panel hides the top 50% of my stash tabs, so i can only read small letters but only 50% of my big letters. is there any way to move the panel higher?
here a small pic from what i mean:
https://picload.org/thumbnail/dgallwrw/griditemcellpanellocation.jpg
https://picload.org/view/dgallwrw/griditemcellpanellocation.jpg.html
changing resolution for the game or size of the panel dont change anything
my resolution is 3840x2160 (tested 1920x1080 and 1280x1024)

in plans rework grid feature where this problem will disappear


User hours minutes only for tracked time displaying instead of Sec2Time format

Feature Description
For the timesheets reporting amount of hours spent for task is more convenient than calendar days, months and years. For example work month has more or less 168 work hours. But hours are always 60 minutes. This is more convenient
for displaying and understanding: workday has 8 hours, workweek has 40 hours.
Screenshots

PR #28312 to implement this.


Add AsyncSimpleConnection trait

Added

Add AsyncSimpleConnection trait, mirroring the SimpleConnection trait provided by Diesel.

This additional trait is helpful for batch executing arbitrary SQL statements that don't produce any final output, namely backend-specific pragmas.
With that said, I wonder if this trait could be done away with entirely, in favor of adding the batch_execute_async() method to the existing AsyncConnection trait instead? The diesel::connection::Connection trait has diesel::connection::SimpleConnection as a trait bound, after all, so you cannot have one without the other. If we go this route, I would be happy to close this PR and open a new one that simply adds the method to AsyncConnection.
CC @mehcode
Sorry to be bugging you with so many pull requests lately. 😅

I'd rather align more than less with Diesel. To ease the burden on the user of having to import all the trait impls, just add a bound to AsyncConnection for AsyncSimpleConnection.

No worries about all the PRs. Thank you for working to make tokio + Diesel a nicer experience.

@mehcode Sounds good to me! I've just added the trait bound and also ran a quick cargo fmt on the codebase before submitting. Hope this looks good to you! Eagerly looking forward to 0.3.0. 😄


Not compatible with unauthorised elastic search

If no elastic search auth details are provided then auth headers should not be passed.
My use case is Elastic Search within a private VPC.
Great project! Really enjoying working with it so far.
Thanks :)

Thanks! Please do send PRs against „develop” branch (I changed base to develop)


JSON Logging

It would be nice if the logging output can be configured to be JSON formatted. This is particularly useful when parsing logs with logging infrastructure.

Great idea! We are happy to accept contribution here!

hi, is anyone working on this or any idea how i can go about it?

See #2432


Assertion failed in jmem_heap_finalize

Jerry version:
Checked revision: 6fe4d06
Build command: ./tools/build.py --clean --debug --profile=es2015-subset --compile-flag=-m32 --system-allocator=on --jerry-libc=off --error-messages=on

OS:
Ubuntu 16.04.2 LTS

Test case:
function $ ( ) { function $ ( ) { switch ( $ ? $ : \u018E ) {} } }

Backtrace:
ICE: Assertion 'JERRY_CONTEXT (jmem_heap_allocated_size) == 0' failed at jerryscript//jerry-core/jmem/jmem-heap.c(jmem_heap_finalize):184.
Error: ERR_FAILED_INTERNAL_ASSERTION

Found by  Fuzzinator with Grammarinator

The issue is confirmed to be valid on latest master c242248b (the hash 6fe4d06 does not seem to be in the master branch) on macOS 10.12.5, too. Moreover, the issue is not ES2015-specific, it can be reproduced by building jerry simply with tools/build.py --clean --debug --error-messages=on. The error message is as in the original report, the backtrace is as follows (as reported by lldb -- ./build/bin/jerry --abort-on-fail issue-1947.js):
    frame #5: 0x0000000100006f9e jerry`jerry_assert_fail(assertion="JERRY_CONTEXT (jmem_heap_allocated_size) == 0", file="jerry-core/jmem/jmem-heap.c", function="jmem_heap_finalize", line=184) at jrt-fatals.c:82
    frame #6: 0x0000000100097eb6 jerry`jmem_heap_finalize at jmem-heap.c:184
    frame #7: 0x00000001000039fe jerry`jmem_finalize at jmem-allocator.c:51
    frame #8: 0x0000000100003531 jerry`jerry_cleanup at jerry.c:180
    frame #9: 0x0000000100001dd6 jerry`main(argc=3, argv=0x00007fff5fbffa70) at main-unix.c:823

I've started working on it.


add db get subcommand

Describe the feature
for running locally and troubleshooting, would be nice to have a get subcommand so we can reth db get Headers --id 2000002 to quickly dump out one record. Related to #612 .
Additional context
No response

Hi @loren! I would pick this one :)

Thanks! I've assigned you to the issue. If you feel stuck, open up a draft PR with your questions and someone will help you. If you are prevented from solving the issue, let me know and I will unassign you :smile:

Hey @gnapoli23 are you still working on this? I'd like to pick it up!

Can I get this one?

Go for it!


Update MANIFEST.in so all resources are installed

Currently when installing via the command:
pip install git+https://github.com/leondz/garak.git only some resources are downloaded. .i.e *.tsv resources are missing

recheck

made some refactoring changes - does this look good to you?
(base) 17:24:38 x1:~/dev/garak [main] $ gh pr checkout 660
Switched to branch 'patch-1'
(base) 17:24:52 x1:~/dev/garak [patch-1] $ flit build
Found 293 files tracked in git                                                                                I-flit.sdist
Built sdist: dist/garak-<IP_ADDRESS>.post1.tar.gz                                                                                I-flit_core.sdist
Copying package file(s) from /tmp/tmpvqsdgzrn/garak-<IP_ADDRESS>.post1/garak                                                      I-flit_core.wheel
Writing metadata files                                                                                I-flit_core.wheel
Writing the record of files                                                                                I-flit_core.wheel
Built wheel: dist/garak-<IP_ADDRESS>.post1-py3-none-any.whl                                                                       I-flit_core.wheel
(base) 17:24:57 x1:~/dev/garak [patch-1] $ cd dist
(base) 17:24:58 x1:~/dev/garak/dist [patch-1] $ tar zxf garak-<IP_ADDRESS>.post1.tar.gz
(base) 17:25:03 x1:~/dev/garak/dist [patch-1] $ cd garak-<IP_ADDRESS>.post1/
(base) 17:25:07 x1:~/dev/garak/dist/garak-<IP_ADDRESS>.post1 [patch-1] $ cd garak/
(base) 17:25:10 x1:~/dev/garak/dist/garak-<IP_ADDRESS>.post1/garak [patch-1] $ find . -name "*tsv"
./resources/book_cloze.tsv
./resources/nyt_cloze.tsv
./resources/guardian_cloze.tsv
./resources/book_cloze_80.tsv
./resources/misp_descriptions.tsv
(base) 17:25:14 x1:~/dev/garak/dist/garak-<IP_ADDRESS>.post1/garak [patch-1] $

Hi @leondz,
Yes it looks good
Thanks


PHP8.0 Migration: decode strings, which were encoded in older php e.g 7.4 version

The sorting functions in PHP 8.0 have been adjusted. this has an effect on the encoding and decoding of texts. So you cannot decode texts that have already been encoded in Php7.4, when you want to update your tool to php8.0.

What behavior do you mean? As I know there is an issue if I would return a boolean instead of an integer in the callback methods of the sorting methods. But I am only using the Spaceship-Operator, which should be safe.
If I am overseeing something, please describe it and I will see what I can do.


Stop VS

The message: No se pudieron rellenar automáticamente las cuentas de Visual Studio Team Services.
Se detectó el error siguiente: TF400813: Resource not available for anonymous access. Client authentication required.

Thanks for your report.
This seems like a crash that happens when retrieving the playlists.

I have debugged the issue, seems like it occurs when trying to use spotWeb = new SpotifyWebAPI();
even if it fails authentication.
Will commit bug fix soon after testing.

Please check out the release 2.0.0 branch: commit 4f33703d should be stable, you can build and install it, and should provide you with the basics of SpotifyRemote.
Although some features are missing it, it way more stable then v1.1.x

I consider this issue closed because of Version 2.0.0 is stable and no longer cause crashes.


Changed postgres healthcheck user, since it was generating a Fatal error in docker logs

With the previous pg_isready -U postgres, the errror "FATAL:  role "postgres" does not exist" was generated

Thanks!


Matcher custom sorting

Is there a way to specify the manner in which the output is to be sorted?
For eg. I want to sort in increasing order of length so that the file/buffer with the shortest possible name is nearest.
Also, does the matcher work across all the different "extensions" or only for files?
For that matter, how do I find out for sure if ctrlp is using the matcher or not?
Thanks

This should work since one of the previous releases.


DefaultMQAdminExtImpl#resetOffsetByTimestampOld complexity is too high, the loop nesting is serious

Please describe the issue you observed:

DefaultMQAdminExtImpl#resetOffsetByTimestampOld complexity is too high, the loop nesting is serious.

Please tell us about your environment:

Java version: 11.0.14
OS name: "mac os x", version: "12.2", arch: "x86_64", family: "mac"

Please describe the feature you are requesting.

Refactor this method, use Map instead of List judgment, reduce loop nesting.

@Kvicii IMO, change return type from List to Map can not avoid loop nesting, because reset each queue offset of each broker is needed.


Toggle Broken for React JSX

Trying to comment JSX tries to use inline Javascript comments:
//<header>

It needs to use block Javascript comments wrapped in curly braces when Vim senses JSX:
{/*<header>*/}

See #17

@alerque pretty sure it can. It's listed as javascript.jsx in my status line.

For those using Vim to program in React please see PR #393 and comments on our support for these filetypes in 41d31b3.


Reading Token information

Hi!
I want to retrieve information about a certain token like serial number, manufacturer etc.
If I select a token with get_token(token_label='somelabel') I only can read token's label with token.label, but all other token data like token manufacturer, model or serial number is empty.
Hasn't it just been implemented or do I forget something?

This has been implemented, but probably you've hit a bug (there's some weird UTF-8 decoding bugs I found with this info on various devices). What HSM are you using?

I'm using SoftHSM2

Oh! They're on token.slot.
e.g.
>>> token.slot.manufacturer_id
'OpenSC Project'

Thank you, that works! But I'm still missing how to get the serial number of the token. Is there also the chance to retrieve this information?

This is token.serial, but it looks like there's a bug.


Update links to point to PyWavelets github repo

Missed a few links to github in the docs. Fixes #90.

good catch


Modified test performance on cleaned up code

I noticed and fixed that the wasm and js update loops were happening no matter what you had selected. Now it only updates the JS Balls when you have the JS options selected, etc.
I also optimized the performance of Ball.js by privatizing as much as I could and removing many this.* calls. I know the point of this experiment was not to create an optimized Ball, but it would be interesting in seeing if rust can benefit from similar simplification of scope chains.
I'm not a rust user (yet) but I would love it if we can see a performance comparison after the Ball.rs gets a small and similar refactor to the one in Ball.js.

Hey @topheman
Sorry this notification slipped through the cracks. I completely agree with you, but it does bring up an interesting point. The tradeoff for the extra performance in my refactor costs more memory since every instance of the object has its own copy of the methods, instead of all sharing a copy of the method on the prototype. I don't think this is language-specific, but rather domain-specific. Trading memory for performance in this case because memory is so bountiful. I still wonder how such a design choice would be made in the Rust code.
Not like I've heard many JS developers measure the memory of the code-side of their projects (it's usually just assets), but that too would be an interesting comparison across JS/Wasm.
Cheers,


Deploy to Kubernetes

📚 Documentation
I'm having a really bad time at trying to deploy a docusaurus built website to Kubernetes, especially when using some trickeries. I wrote a Dockerfile like:
FROM node:lts AS build
WORKDIR /app
COPY package.json package-lock.json ./
RUN npm install
COPY . ./
RUN npm run build

FROM node:lts
COPY --from=build --chown=node:node /app /app
USER node
ENV PATH="/app/node_modules/.bin:$PATH"
CMD cd /app && \
    exec docusaurus serve --build --port 80 --host <IP_ADDRESS>

With a docusaurus.config.js like so:
module.exports = {
  url: process.env.TARGET_URL ?? "http://localhost/",
  baseUrl: process.env.PREFIX ?? "/"
}

My goal is to be able to customize the url and baseUrl fields, so I can adjust them properly depending on where I'm deploying using Helm and Ingress.
But it never works:

I even tried https://github.com/facebook/docusaurus/issues/3291#issuecomment-674929740 through:
CMD cd /app && \
    set -x && \
    exec docusaurus serve --dir "build/$(basename "${PREFIX}")" --build --port 80 --host <IP_ADDRESS>

Which produces an output like so:

(disregard the port 81/80 mapping. I'm running in a SSH server and I'm forwarding the port 80 to my host's 81).
But the result is the same when I browse http://localhost:81/ui:

Once I can finally get this working, I can happily submit a PR for enhancing the docs. :)
Have you read the Contributing Guidelines on issues?
Yes.

Ok, I figured out what's wrong. The following works:
CMD set -x && \
    cd /app && \
    docusaurus build && \
    if [ -n "$PREFIX" ]; then \
        mv -f build build.old && \
        mkdir -p build && \
        mv -f build.old "build/$PREFIX"; \
    fi && \
    exec docusaurus serve --port 80 --host <IP_ADDRESS>

But probably it would be better to fix the serve command to respect the baseUrl field.

Afaik the simple http server we use for serve does not provide any option to respect the baseUrl. If you want it to be served from /baseUrl/, yu have to put the static files in website/build/baseUrl somehow.
Related to https://github.com/facebook/docusaurus/issues/3291
There's probably a solution to make it support the baseUrl but it's not yet implemented

Closing since serve command now respects the base URL (see https://github.com/facebook/docusaurus/pull/4924).


fix: demo empty whenclose all seed

Codecov Report

Merging #77 (df1a80e) into master (c3087d5) will decrease coverage by 0.13%.
The diff coverage is 0.00%.

:exclamation: Current head df1a80e differs from pull request most recent head 9b1fc86. Consider uploading reports for the commit 9b1fc86 to get more accurate results

@@            Coverage Diff             @@
##           master      #77      +/-   ##
==========================================
- Coverage   66.86%   66.73%   -0.14%
==========================================
  Files         258      258
  Lines        1527     1530       +3
  Branches      242      245       +3
==========================================
  Hits         1021     1021
- Misses        505      508       +3
  Partials        1        1

Impacted Files
Coverage Δ

src/ThemeEditor.tsx
0.00% <0.00%> (ø)

src/token-info/TokenRelation.ts
0.00% <0.00%> (ø)

src/token-panel-pro/ColorTokenContent.tsx
0.00% <0.00%> (ø)

Continue to review full report at Codecov.

Legend - Click here to learn more
Δ = absolute <relative> (impact), ø = not affected, ? = missing data
Powered by Codecov. Last update c3087d5...9b1fc86. Read the comment docs.


trying to move water into building

ok. for sure i'm missing something but after 2h of fighting i gave up.
i try to move water into building. and i'm failing. i connect pipe from the outside but i only have out icon inside. my belt had green in.
pic outside

pic of inside

pipeline is like 50 long rn as it's relatively close to lake

today update fixed it. now when i select arrow and hit R it changes state


Add unit tests for deployableByOLM check + refactoring

Add unit tests for deployableByOLM check + refactoring
Fixes #198

@bcrochet This is the expected behavior when the second test is running. The test ensures that Validate() returns an error if the check is unable to fetch the Subscription.status.installedCSV.

@bcrochet This is the expected behavior when the second test is running. The test ensures that Validate() returns an error if the check is unable to fetch the Subscription.status.installedCSV.

It just hangs for a bit. I don't think we want to introduce an unnecessary delay to the unit tests.

@bcrochet This is the expected behavior when the second test is running. The test ensures that Validate() returns an error if the check is unable to fetch the Subscription.status.installedCSV.

It just hangs for a bit. I don't think we want to introduce an unnecessary delay to the unit tests.

Yes. The timeout duration (which is 180 sec) is hard coded and the unit test is hanging until the context is timed-out. Parametrizing the timeout duration might be an overkill.

@bcrochet This is the expected behavior when the second test is running. The test ensures that Validate() returns an error if the check is unable to fetch the Subscription.status.installedCSV.

It just hangs for a bit. I don't think we want to introduce an unnecessary delay to the unit tests.

Yes. The timeout duration (which is 180 sec) is hard coded and the unit test is hanging until the context is timed-out. Parametrizing the timeout duration might be an overkill.

I don't want to introduce a 180 second pause on every unit test run. That will add up very quickly throughout the day.

@bcrochet This is the expected behavior when the second test is running. The test ensures that Validate() returns an error if the check is unable to fetch the Subscription.status.installedCSV.

It just hangs for a bit. I don't think we want to introduce an unnecessary delay to the unit tests.

Yes. The timeout duration (which is 180 sec) is hard coded and the unit test is hanging until the context is timed-out. Parametrizing the timeout duration might be an overkill.

I don't want to introduce a 180 second pause on every unit test run. That will add up very quickly throughout the day.

Ok.. Are you suggesting to leveraging viper to pass the timeout duration?

@bcrochet This is the expected behavior when the second test is running. The test ensures that Validate() returns an error if the check is unable to fetch the Subscription.status.installedCSV.

It just hangs for a bit. I don't think we want to introduce an unnecessary delay to the unit tests.

Yes. The timeout duration (which is 180 sec) is hard coded and the unit test is hanging until the context is timed-out. Parametrizing the timeout duration might be an overkill.

I don't want to introduce a 180 second pause on every unit test run. That will add up very quickly throughout the day.

Ok.. Are you suggesting to leveraging viper to pass the timeout duration?

Not at all. There are other ways. I'm testing something out now. I'm not sure of the solution quite yet.

@bcrochet This is the expected behavior when the second test is running. The test ensures that Validate() returns an error if the check is unable to fetch the Subscription.status.installedCSV.

It just hangs for a bit. I don't think we want to introduce an unnecessary delay to the unit tests.

Yes. The timeout duration (which is 180 sec) is hard coded and the unit test is hanging until the context is timed-out. Parametrizing the timeout duration might be an overkill.

I don't want to introduce a 180 second pause on every unit test run. That will add up very quickly throughout the day.

Ok.. Are you suggesting to leveraging viper to pass the timeout duration?

Not at all. There are other ways. I'm testing something out now. I'm not sure of the solution quite yet.

Make a var at the top of the file (var csvTimeout time.Duration = 180), and override it in the test.

@bcrochet This is the expected behavior when the second test is running. The test ensures that Validate() returns an error if the check is unable to fetch the Subscription.status.installedCSV.

It just hangs for a bit. I don't think we want to introduce an unnecessary delay to the unit tests.

Yes. The timeout duration (which is 180 sec) is hard coded and the unit test is hanging until the context is timed-out. Parametrizing the timeout duration might be an overkill.

I don't want to introduce a 180 second pause on every unit test run. That will add up very quickly throughout the day.

Ok.. Are you suggesting to leveraging viper to pass the timeout duration?

Not at all. There are other ways. I'm testing something out now. I'm not sure of the solution quite yet.

Make a var at the top of the file (var csvTimeout time.Duration = 180), and override it in the test.

With this, this PR  looks good.


Spread syntax for query/mutation args

First of all I want to say that this is some awesome work! I've been hunting for something like it and none of the other solutions I've seen feel anywhere near as clean as this project. I feel like the repetition of GQL is one of the things that have hit me the hardest when defining schemas. So all in all thank you for this awesome project!
With regards to the project, I was wondering if there are any plans to support the use of the spread syntax inside of queries/mutation args. Currently I have a query and I would like to be able to pass in some pagination args that I have defined in an input, but I believe this is not supported.
  getThings(
    q: String!
    ..._PaginationInput
  ): [Thing!]!

Hi @RobertVillalba! glad you find this library useful!
It's an interesting suggestion, but I'm not sure about the cost/benefit of adding the spread operator in input arguments. It would lead to some clunky edge cases, like what if you have directives in the input that are just meant to be used in input types? or what if the type you are spreading implements an interface? or what if there is a top level directive in the input, should it be lost?
The truth is that structural entities (inputs, types...) and arguments are of significantly different nature, I also don't think that other languages with spread operators support this, I'm not sure about their specific reasons though.
I think it's worth deferring this as much as possible until enough use-cases are gathered in order to know what is the best way of implementing this.

Hmm ok those are all fair points, perhaps what I want is something more simple. So let me preface this all with the fact that I'm fairly new to GQL and there is a great deal of things I'm not going to be familiar with or aware of. My background is more closely aligned with TS so I'm probably just wanting to bring some of those features to GQL. Not that TS has this exact same thing but, here is what I think would make GQL easier and would be great to see graphqxl allow.
(a bit of a detour follows, but promise it will get back to this subject)
I'm not a big fan of the separation between inputs and types. I sort of get why inputs have to be separated, but a lot of times I find that I'm just using simple inputs and types that could have been used interchangeably if it was not for GQL limitations. This lib greatly helps with this issue, but I find that it doesn't cover the case where I have inputs inside of inputs. I can't spread those so I need some repetition
input Thing1Input {
  "some nice docs"
  hello: String!
}

input Thing2Input {
  "some more nice docs"
  world: String!
  "some even nicer docs"
  thing1: Thing1Input!
}

type Thing2 {
  ...Thing2Input
}

To make the above example work, we'd have to make a type Thing1 and then duplicate the doc string for thing1 inside of Thing2 and pretty much also copy the thing1 line. It would be great to add some sort of type like a template that could just be used for both. The obvious restriction is that template would only be allowed to contain simple data (w.e. that might be) and that it would generate a type and an input. Allowing us to write something like
template Thing1 {
  "some nice docs"
  hello: String!
}

template Thing2 {
  "some more nice docs"
  world: String!
  "some even nicer docs"
  thing1: Thing1!
}

that would generate
input Thing1Input {
  "some nice docs"
  hello: String!
}

input Thing2Input {
  "some more nice docs"
  world: String!
  "some even nicer docs"
  thing1: Thing1Input!
}

type Thing1 {
  "some nice docs"
  hello: String!
}

type Thing2 {
  "some more nice docs"
  world: String!
  "some even nicer docs"
  thing1: Thing1!
}

If we place a restriction that templates must be simple then we can perhaps also allow spreading them inside of functions as well since it might eliminate some of the mentioned concerns?
Sorry I know that was fairly drawn out and perhaps a separate ticket, but they seemed somewhat related.


[R3] Divider component

https://taskman.eionet.europa.eu/issues/150079
Storybook: https://62713c57bcbdf4004a3d99b8-efzlmwtdza.chromatic.com/
Docusaurus: https://ci.eionet.europa.eu/job/volto-addons/job/volto-eea-design-system/job/PR-170/12/volto-eea-design-system/

@eea-jenkins build all

@eea-jenkins build all

@eea-jenkins build all

@eea-jenkins build all

Considering to change default divider color. Please to do not merge yet.

@eea-jenkins build all

@eea-jenkins build all

Testimonial divider updated according to @avoinea comment.


epoxy: 1.5.2 -> 1.5.3

Motivation for this change
Fix broken runtime on darwin (anholt/libepoxy#176)
Things done

[ ] Tested using sandboxing (nix.useSandbox on NixOS, or option sandbox in nix.conf on non-NixOS)
Built on platform(s)

[ ] NixOS
[x] macOS
[ ] other Linux distributions

[ ] Tested via one or more NixOS test(s) if existing and applicable for the change (look inside nixos/tests)
[ ] Tested compilation of all pkgs that depend on this change using nix-shell -p nox --run "nox-review wip"
[ ] Tested execution of all binary files (usually in ./result/bin/)
[ ] Determined the impact on package closure size (by running nix path-info -S before and after)
[x] Fits CONTRIBUTING.md.

Success on aarch64-linux (full log)
Attempted: epoxy
Partial log (click to expand)
/nix/store/d06lcfd1bg8layspx2y8snwjwz78p223-epoxy-1.5.3

Success on x86_64-linux (full log)
Attempted: epoxy
Partial log (click to expand)
strip is /nix/store/j13c9z0z495wxdc97if8z451x53m1j25-binutils-2.30/bin/strip
stripping (with command strip and flags -S) in /nix/store/ggwq7972mxvff296gwl6awkrp2g1zcds-epoxy-1.5.3/lib
patching script interpreter paths in /nix/store/ggwq7972mxvff296gwl6awkrp2g1zcds-epoxy-1.5.3
checking for references to /build/ in /nix/store/ggwq7972mxvff296gwl6awkrp2g1zcds-epoxy-1.5.3...
shrinking RPATHs of ELF executables and libraries in /nix/store/5mb89npjm12m0n54wxylj0wnp4gipjf5-epoxy-1.5.3-dev
strip is /nix/store/j13c9z0z495wxdc97if8z451x53m1j25-binutils-2.30/bin/strip
stripping (with command strip and flags -S) in /nix/store/5mb89npjm12m0n54wxylj0wnp4gipjf5-epoxy-1.5.3-dev/lib
patching script interpreter paths in /nix/store/5mb89npjm12m0n54wxylj0wnp4gipjf5-epoxy-1.5.3-dev
checking for references to /build/ in /nix/store/5mb89npjm12m0n54wxylj0wnp4gipjf5-epoxy-1.5.3-dev...
/nix/store/ggwq7972mxvff296gwl6awkrp2g1zcds-epoxy-1.5.3

Timed out, unknown build status on x86_64-darwin (full log)
Attempted: epoxy
Partial log (click to expand)
cannot build derivation '/nix/store/5g0ak36yzkpmv5fs03rlz9f8qfb48kpq-apple-framework-CoreVideo.drv': 7 dependencies couldn't be built
cannot build derivation '/nix/store/r2r13zh8pwgkrvz3q603h0zqb2bjxj35-apple-framework-Foundation.drv': 7 dependencies couldn't be built
cannot build derivation '/nix/store/mqwij8dzsgvgwklp0s5hrrjzngmi2gkl-python2.7-Mako-1.0.7.drv': 10 dependencies couldn't be built
cannot build derivation '/nix/store/8c9qa3mvx7a395shjwyzrkd4x3bamgic-apple-framework-QuartzCore.drv': 6 dependencies couldn't be built
cannot build derivation '/nix/store/32pyq3i5yvz6iglfnx74vhybmwc8sa6k-apple-framework-Carbon.drv': 9 dependencies couldn't be built
cannot build derivation '/nix/store/78a2c50385704yr479yl8607sk1638l4-apple-lib-Xplugin.drv': 10 dependencies couldn't be built
cannot build derivation '/nix/store/7rimbqqhpd5qnwdpm91a8ja7j5yb3p21-mesa-noglu-18.2.5.drv': 31 dependencies couldn't be built
cannot build derivation '/nix/store/bch8mxcyzznj9yws95jkq8p2x4ai66v3-libGL-1.0.0.drv': 4 dependencies couldn't be built
cannot build derivation '/nix/store/ablfx4yd7h4walfq14bfn8k9sc36wiz4-epoxy-1.5.3.drv': 8 dependencies couldn't be built
error: build of '/nix/store/ablfx4yd7h4walfq14bfn8k9sc36wiz4-epoxy-1.5.3.drv' failed

@GrahamcOfBorg build epoxy mesa_noglu

Changes between the versions: https://github.com/anholt/libepoxy/compare/1.5.2...1.5.3


fix NPE in percentile aggregation function

NPE was occurring for cases when the queried shards had no
records. The reason of NPE was an attempt of serialization of
the new state which remains null until the first call of the iterate
method. If shards have no rows the iterate won't be called and the
state won't be initialized.
This commit also adds logic for handling some corner cases.
percentile(col, [.85, null]) -> [result, null]
percentile(col, null) -> null

Current coverage is 81.02% (diff: 100%)

Merging #4400 into master will increase coverage by 0.01%

@@             master      #4400   diff @@
==========================================
  Files          1302       1302
  Lines         44049      44053     +4
  Methods           0          0
  Messages          0          0
  Branches       7010       7010
==========================================
+ Hits          35682      35692    +10
+ Misses         5690       5673    -17
- Partials       2677       2688    +11

Powered by Codecov. Last update 7250345...ab5ff7f

@dobe please review

https://github.com/crate/crate/pull/4405


Dev/button rework2

See this for an example of how this works.

I'm not against merging this, however I would personally use the collector api in most use cases, as it offers a balance of abstraction and escape hatches.
Some examples include:

A built-in filter predicates in the collector.
Using function references for the collector handlers.
Uses raw components so it's as customizable as possible.

Fair enough. This doesn't break any existing code so you can feel free to use the collector method if you prefer that. If nobody but me prefers this API, though, we probably should not add it. Filter predicates are a good point. I'm not sure I understand the function references; you can just do onClick: myref with my API.

We should get input from @VijayStroup and @Armintron on what method they prefer

Could I get a summary of what this PR does? I haven't worked with buttons or anything so I'm not sure what's good/bad.

I’ll post two examples of both of the implementations later.
Thanks, Suneet Tipirneni

On Jul 25, 2021, at 1:36 PM, Armin @.***> wrote:

Could I get a summary of what this PR does? I haven't worked with buttons or anything so I'm not sure what's good/bad.
—
You are receiving this because your review was requested.
Reply to this email directly, view it on GitHub, or unsubscribe.

Could I get a summary of what this PR does? I haven't worked with buttons or anything so I'm not sure what's good/bad.

But TLDR; if your used to emoji reaction buttons, then the discord collectors act almost exactly the same. This PR takes a more abstract approach.

As long as it is easier for the end developer or anyone new that might join the team in creating additions, I am all for it.

As long as it is easier for the end developer or anyone new that might join the team in creating additions, I am all for it.

+1

My Thoughts on the Structure of Commands
There has been a great deal of debate between @suneettipirneni and I about the best way to build a Command interface for command authors. I am documenting my thoughts here for clarity and final consideration. The final decision is of course, his, but I want to put my best foot forward and make my arguments unambiguous.
My Ideal Command
Here is an example of what I see as the ideal command interface:
function generateVibeEmbed(sender: User, recipient: User): MessageEmbed {
  // ...
}
const VibeCommand: Command = {
  name: "vibe",
  description: "Performs a vibe check on the given user.",
  options,
  permissionHandler: inChannelNames(Channels.bot),
  async run({ interaction: { options, user, followUp, defer }, registerUI }) {
    const sender = user;
    const userToVibeCheck = options.get("user")?.user;

    // Show that the bot is thinking.
    await defer();

    const ui: UIComponent = new DispatchButton({
      style: "PRIMARY",
      label: "Recheck",
      async onClick({ deferUpdate, editReply }) {
        await deferUpdate();
        await editReply({
          embeds: [generateVibeEmbed(sender, userToVibeCheck ?? sender)],
        });
      },
    });

    await followUp({
      embeds: [generateVibeEmbed(sender, userToVibeCheck ?? sender)],
      components: registerUI(ui),
    });
  },
};

Why do I think this is good? First, the Command is represented as the most minimal structure possible. Object property mappings like name: 'vibe' are pretty much as succinct as they can be. There is no simpler way to express the name, description, or options of the command.
Second, the run() function definition is ideal: the dependencies of the function are extremely clear.
async run({ interaction: { options, user, followUp, defer }, registerUI }) {

From this line alone, I can easily tell that this command needs the following: the arguments to the command, the user who initiated the interaction, a way to follow up, a way to defer the follow up, and it needs to register a UI. This is useful information for quickly scanning code or trying to understand how the command works without reading every single line.

If it seems odd to you to pass in functions as a argument, consider that this is an extremely common practice in functional programming languages and modern frameworks like React. Something as simple as an onClick prop in a React component or using Array.prototype.map is an example of how you might be doing this every day. First class functions are a modern programming language feature that applies to many different designs.

Lastly, notice the generateVibeEmbed(sender, recipient) function. It is simple to understand from its function signature alone and, more importantly, easily reusable. If we later wanted to generate vibe embeds for another command, it would be easy to copy and paste this function to a new file and import it here and into the new command, with no other changes.
A Less Good Command
const row = singleButtonRow({
  label: "Recheck",
  customId: "vibeButton",
  style: "PRIMARY",
});

class VibeCommand extends Command {
  readonly name = "vibe";
  readonly description = "Performs a vibe check on the given user.";
  readonly options = options;
  readonly permissionHandler = inChannelNames(Channels.bot);

  constructor(client) {
    super(client);
  }

  async run(interaction: CommandInteraction) {
    // Show that the bot is thinking.
    await interaction.defer();

    this.sendVibeEmbed(interaction);
  }
  sendVibeEmbed(interaction: CommandInteraction): MessageEmbed {
    // ...
    const message = (await interaction.followUp({
      embeds: [this.generateVibeEmbed(interaction)],
      fetchReply: true,
      components: [row],
    })) as Message;

    const collector = message.createMessageComponentCollector({
      componentType: "BUTTON",
    });
    collector.on("collect", async (i) => {
      await i.deferUpdate();
      await i.editReply({ embeds: [this.generateVibeEmbed(interaction)] });
    });
  }
}

I want to make clear that I don't think this is an absolutely terrible design. It shares many of the benefits of my preferred design, including a declarative syntax for the name and other fields and strong type checking to ensure command authors include all the necessary properties. However, I do think this is a worse design. Why?
Murkier Dependencies
The most important difference is that the dependencies are murkier. Compare the lines below:
// good
async run({ interaction: { options, user, followUp, defer }, registerUI }) {
// less good
async run(interaction: CommandInteraction) {

It is less clear from the function signature what the command needs. This may seem like a small difference, but I find that writing code with more explicit dependencies is valuable for later comprehension. The bottom example is less clear, and also less testable. If I were going to try to test the top example, it is more clear what I need to mock out than with the bottom example.
Single Responsibility Principle Violation
A similar problem exists with this implementation of generateVibeEmbed(). In this version, sendVibeEmbed() is a method on the class and pulls its dependencies directly off of the interaction. Why is this bad? In reality, we know it only needs two users in order to work properly. This "overpassing" of the entire interaction hides the true dependencies of the function and also makes it more difficult to test sendVibeEmbed(). We now need to mock an entire interaction and also the sending of the embed.
This is a violation of the single responsibility principle, and the weaknesses of this approach become clear after some examination.
Why is this actually bad?
sendVibeEmbed() is less reusable than generateVibeEmbed(). If we wanted to use it in a different command, we would have to refactor it to a free function or duplicate code. It is also less flexible in terms of what user it will generate the embed for, since it always pulls them off of the interaction.
For instance, if I had fetched a user from a leaderboard and wanted to check their vibe, I can't do that with sendVibeEmbed() because it asks for an interaction, not a user. Also, the code will break because sendVibeEmbed() requires that interaction.defer() is called first since it uses interaction.followUp().
Of course, this can be addressed with refactoring, but with the first design, you got all of this built in for free.
Accidental Complexity
Last, and also least, there is additional accidental complexity from the readonlys, constructor, and especially the Collector. The customId is also not truly essential to express what the command author wants and is accidental complexity.
// in the future
constructor(client, storage, /* ... */) {
  super(client, storage);
}

Due to the inheritance hierarchy, we're stuck passing in Client and any future new dependencies into every single command, even if they don't need those dependencies. We can use destructuring to fix this, but it isn't clear to me how this is better than just passing these dependencies directly into run().
// why not just do this with run()?
constructor({ client, storage, /* ... */ }) {
  super();
  this.client = client;
  this.storage = storage;
}

All in all, the class-based approach adds some dead weight that doesn't really add any value to the command, but increases the amount of time necessary to grok the code.
Class-based Command, pt 2
Now, one can argue that the second example isn't fair. After all, you can implement the class-style Command in a similar way to the first example and eliminate many of the issues. I agree. However, some of the issues, like useless readonlys (or publics) simply aren't possible with the first approach. Using the first approach also forces you to explicitly pass the dependencies you need and makes it harder to mangle your state and logic as in the second example with sendVibeEmbed(). Nonetheless, let's consider a better class-based implementation:
class VibeCommand extends Command {
  name = "vibe";
  description = "Performs a vibe check on the given user.";
  options = options;
  permissionHandler = inChannelNames(Channels.bot);

  constructor({ client }) {
    super();
    this.registerUI = client.registerUI;
  }

  async run({ options, user, defer, followUp }) {
    const sender = user;
    const userToVibeCheck = options.get("user")?.user;

    // Show that the bot is thinking.
    await defer();

    const ui: UIComponent = new DispatchButton({
      style: "PRIMARY",
      label: "Recheck",
      async onClick({ deferUpdate, editReply }) {
        await deferUpdate();
        await editReply({
          embeds: [this.generateVibeEmbed(sender, userToVibeCheck ?? sender)],
        });
      },
    });

    await followUp({
      embeds: [this.generateVibeEmbed(sender, userToVibeCheck ?? sender)],
      fetchReply: true,
      components: this.registerUI(ui),
    });
  }
  generateVibeEmbed(sender: User, recipient: User): MessageEmbed {
    // ...
  }
}

How is it better?
This is better from a design standpoint for a number of reasons. There's less verbosity, and better clarity at the function signatures about what each method needs to function. It largely looks like a class-based mirror of the first approach.
Why is it still worse?
The main reasons I don't like this solution are primarily related to the use of class.
this is bad
First, we need to have a constructor and type this several times. This is not a huge deal, but why not cut down on this bloat if it doesn't provide any benefit? I can't find a convincing reason why this is better than just passing the necessary dependencies straight into run().
Less Reusable
Second, generateVibeEmbed() is still not as reusable as when it was a free function. If we ever try to extract it to its own file, we will have to update the callsites not to use this in order to use the imported version.
// ugh, compile errors everywhere
    this.generateVibeEmbed();
//  ^^^^

This isn't so terrible - in fact, our IDE might even offer to automatically do this for us - but why not just write it as a free function from the start? It is apparent that even with a class based approach, free functions are sometimes a strict upgrade from methods.
this is really bad
Finally, this code has a serious bug.
async onClick({ deferUpdate, editReply }) {
  await deferUpdate();
  await editReply({
    embeds: [this.generateVibeEmbed(sender, userToVibeCheck ?? sender)],
  });
},

The above code breaks on the line with this because that this does not refer to the VibeCommand class. You can get around this by using an arrow function or something like const that = this, but it is simpler just to sidestep these issues entirely by not using a class.
A class with 1 method and a constructor is just a function
My final point about this example is that Commands in their current state are a poor candidate for a class. generateVibeEmbed() really should just be a free function as discussed to avoid the various pitfalls of putting it as a method. That leaves only two methods on the class, one of which is the constructor.
// why do this...
class VibeCommand extends Command {
  // ...
  constructor({ client }) {
    super();
    this.registerUI = client.registerUI;
  }
  // ...
  async run({ options, user, defer, followUp }) {
    // ...
  }
}

The simplifying idea is discussed at length here if you can ignore the overly inflammatory title.
// ...when you could do this!
const VibeCommand: Command = {
  // ...
  async run({ interaction: { options, user, followUp, defer }, registerUI }) {
    // ...
  }
}

In short, however, a class with only two methods, one of which is the constructor, is usually better represented by a function that just takes all of the arguments together. After all, a class is a place to aggregate multiple methods that operate on shared state; if there is only one true method (in our case run()), then there isn't any shared state. All you need is a function.
Conclusion
I've tried my best to point out why I believe my proposed changes give the most flexible and maintainable design. I'm not against classes or OOP in general, but I don't believe they are the best choice for Commands. Using a class opens the door to several potential issues, while providing no real value over the first approach. A combination of free functions and argument destructuring will encourage command authors to use a more testable and maintainable design, which benefits us all in the end.
@suneettipirneni @Armintron @VijayStroup Lmk what you think about this

Really indifferent on the structure; I can do whatever as long as documentation is maintained. So in that sense, whatever is easier to document is my choice.

In all honestly, the documentation is basically "look at another command similar to what you want to do" or ask someone who knows what they're doing either way.

Murkier Dependencies
The most important difference is that the dependencies are murkier. Compare the lines below:

// good
async run({ interaction: { options, user, followUp, defer }, registerUI }) {
// less good
async run(interaction: CommandInteraction) {

It is less clear from the function signature what the command needs. This may seem like a small difference, but I find that writing code with more explicit dependencies is valuable for later comprehension.

As indicated by "I find", this is purely an opinion. If you're having issues finding dependencies look at the constructor, or use the "find usages" tool available in most modern IDE's.
I'm failing to recognize why this matters in our capacity? I don't care about this function's dependencies, and I don't want to care, that's the point. Abstraction is about pulling out the logic you care about and leaving the rest to the internal implementation/buisness logic.

If I were going to try to test the top example, it is more clear what I need to mock out than with the bottom example.

You don't know that just looking at the function signature, what the underlying functions that are called by the functions you pass in. Functions call other functions, if you want a full mock you'd still have to dig into the dependencies of your dependencies, it's not really an improvement over anything.
In terms of testing, OOP has dependency injection via the constructor for that. Also, why would I change the registerUI() function for testing? If I change the implementation I'm not testing the implementation that's actually being used, that IOC is useless. Also why can't I just make a mock object of the interaction?
In fact, that brings me to my next point a lot of these parameters can be changed, but in reality you won't accomplish anything by changing their implementation. registerUI() uses a method on the instance of Client, you can't accomplish any more meaningful changes without changing the Client#addButtonListener method itself. Hence adding to my point of making them instance methods.

Single Responsibility Principle Violation
A similar problem exists with this implementation of  generateVibeEmbed(). In this version,  sendVibeEmbed()  is a method on the class and pulls its dependencies directly off of the interaction. Why is this bad? In reality, we know it only needs two users in order to work properly. This "overpassing" of the entire  interaction  hides the true dependencies of the function and also makes it more difficult to test  sendVibeEmbed(). We now need to mock an entire  interaction  and also the sending of the embed.

This is a violation of the  single responsibility principle, and the weaknesses of this approach become clear after some examination.

Why is this actually bad?

sendVibeEmbed()  is less reusable than  generateVibeEmbed(). If we wanted to use it in a different command, we would have to refactor it to a  free function  or duplicate code. It is also less flexible in terms of what user it will generate the embed for, since it always pulls them off of the interaction.

For instance, if I had fetched a user from a leaderboard and wanted to check their vibe, I can't do that with  sendVibeEmbed()  because it asks for an interaction, not a user. Also, the code will break because  sendVibeEmbed()  requires that  interaction.defer()  is called first since it uses  interaction.followUp().

Of course, this can be addressed with refactoring, but with the first design, you got all of this built in for free.

The issues mentioned above seem to have a trivial solution, (that happens to be the same solution for the first design):
// Remedies issues from above!
function generateVibeEmbed(sender: User, reciever: User) {}

class MyCommand extends Command {
  async run(interaction) {
    ...
    generateVibeEmbed(sender, reciever);
  }
}

Accidental Complexity

Last, and also least, there is additional  accidental complexity  from the  readonlys, constructor, and especially the Collector. The  customId  is also not truly essential to express what the command author wants and is accidental complexity.

The client is necessary for registerUI() and the storage properties. There's nothing accidental there.
// in the future
constructor(client, storage, /* ... */) {
  super(client, storage);
}

Due to the inheritance hierarchy, we're stuck passing in  Client  and any future new dependencies into every single command, even if they don't need those dependencies. We can use destructuring to fix this, but it isn't clear to me how this is better than just passing these dependencies directly into  run().

The inheritance chain ends with the Command class. Any other orthogonal classes created have as much freedom as they want.
// why not just do this with run()?
constructor({ client, storage, /* ... */ }) {
  super();
  this.client = client;
  this.storage = storage;
}

Realistically, client would store storage, and the constructor would have only one argument. Even better, is the fact that the constructor declaration be omitted in many commands where they don't need to initialize any data.

All in all, the class-based approach adds some dead weight that doesn't really add any value to the command, but increases the amount of time necessary to grok the code.

Class-based  Command, pt 2
How is it better?
This is better from a design standpoint for a number of reasons. There's less verbosity, and better clarity at the function signatures about what each method needs to function. It largely looks like a class-based mirror of the first approach.
Why is it still worse?
The main reasons I don't like this solution are primarily related to the use of  class.

this  is bad

First, we need to have a constructor and type  this  several times. This is not a huge deal, but why not cut down on this bloat if it doesn't provide any benefit? I can't find a convincing reason why this is better than just passing the necessary dependencies straight into  run().

Using this sounds a lot easier than having to constantly pass the same dependencies into functions over and over again. Every method that depends on the client needs to have it injected in the function.
"Just use a partially applied function!"
You're going to go through that much effort just to avoid typing this? A class gives you this behavior for free!

Less Reusable

Second,  generateVibeEmbed()  is still not as reusable as when it was a free function. If we ever try to extract it to its own file, we will have to update the callsites not to use  this  in order to use the imported version.

// ugh, compile errors everywhere
    this.generateVibeEmbed();
//  ^^^^

This isn't so terrible - in fact, our IDE might even offer to automatically do this for us - but why not just write it as a free function from the start? It is apparent that even with a class based approach, free functions are sometimes a strict upgrade from methods.

Good question! If you wanna use classes and want a super reusable-stateless function, then why not create a top level function or a static function, and have your class reference it, like my example for generateVibeEmbed? Stateless utility functions are very common in OOP.

My final point about this example is that  Commands in their current state are a poor candidate for a class.  generateVibeEmbed()  really should just be a free function as discussed to avoid the various pitfalls of putting it as a method. That leaves only two methods on the class, one of which is the constructor.

// why do this...
class VibeCommand extends Command {
  // ...
  constructor({ client }) {
    super();
    this.registerUI = client.registerUI;
  }
  // ...
  async run({ options, user, defer, followUp }) {
    // ...
  }
}

registerUI would realistically be an instance method, not a variable, meaning there's more than one method in this class. Even better this can be assigned in Command, greatly reducing the amount of code in the example.

In short, however, a class with only two methods, one of which is the constructor, is usually better represented by a function that just takes all of the arguments together. After all, a class is a place to aggregate multiple methods that operate on shared state; if there is only one true method (in our case  run()), then there isn't any shared state. All you need is a function.

Ok but what happens when I want multiple functions that use the client? For example, what if I want to add an enable and disable method:
class MyCommand extends Command {
  enable() {}
  disable() {}
  isEnabled() {}
}

I need shared state to keep track of that, if I'm left with an object, I can't expand much beyond it.
"Just pass it into the run()function!"
So at what point does this ever-growing object being put into the run function become a god object?
Also here's some food for though, if I only destructure one of the many fields in the object given by run isn't that "hiding" dependencies? Because the other fields are still populated with values, you just aren't accessing them. This isn't reactjs where components sometimes only provide certain subset values in an object. In our case all of the values in object would be filled in all the time. So whether the dependency is hidden or not makes no difference. It's no different from the choice of accessing a field or not accessing a field. In terms of readability well that's subjective.

As indicated by "I find", this is purely an opinion. If you're having issues finding dependencies look at the constructor, or use the "find usages" tool available in most modern IDE's.

I'm failing to recognize why this matters in our capacity? I don't care about this function's dependencies, and I don't want to care, that's the point. Abstraction is about pulling out the logic you care about and leaving the rest to the internal implementation/buisness logic.

You're right, I'm realizing that this is just a preference on my part.

You don't know that just looking at the function signature, what the underlying functions that are called by the functions you pass in. Functions call other functions, if you want a full mock you'd still have to dig into the dependencies of your dependencies, it's not really an improvement over anything.

My point here is that you could avoid mocking out the unused parts of the object. Even so, this is still largely just my preference so I'll concede this.

In terms of testing, OOP has dependency injection via the constructor for that. Also, why would I change the registerUI() function for testing? If I change the implementation I'm not testing the implementation that's actually being used, that IOC is useless. Also why can't I just make a mock object of the interaction?

Again, you're right, you could just make a mock object of the interaction.

In fact, that brings me to my next point a lot of these parameters can be changed, but in reality you won't accomplish anything by changing their implementation. registerUI() uses a method on the instance of Client, you can't accomplish any more meaningful changes without changing the Client#addButtonListener method itself. Hence adding to my point of making them instance methods.

I'm a bit uncertain what you mean by this - there is no Client#addButtonListener method? If you mean that registerUI depends on Client internals to work, which is true (it does use a map in Client), what I prefer about just passing registerUI is that if we wanted to cut Client out of the loop later we could, and commands wouldn't have to know about this change. The map would just have to live somewhere else.
For instance, if we switched our bot to work via a serverless webhook with no Client object, we run into issues injecting client via the constructor because we don't have one. If we keep registerUI separate, we would still be able to provide a registerUI function after moving the map somewhere else, and no code inside of commands would have to change. That's why it seems to me that injecting the function is more flexible than injecting the Client.

The issues mentioned above seem to have a trivial solution, (that happens to be the same solution for the first design):

This is 100% true and I promise I was not trying to set up a straw man here. My experience is that when people have a class, they tend to favor attaching methods and often don't consider a free function. I tend to see a class as opening the door to these kinds of poor designs. For some reason, people don't make the same mistake without a class. We can definitely just be strict about making sure people use free functions appropriately in code reviews and then the solutions are equivalent.

The client is necessary for registerUI() and the storage properties. There's nothing accidental there.

Again, because of the potential to switch to a webhook, I don't agree with this. Storage and registerUI() and the associated machinery can exist even if there's no client object.

The inheritance chain ends with the Command class. Any other orthogonal classes created have as much freedom as they want.
Realistically, client would store storage, and the constructor would have only one argument. Even better, is the fact that the constructor declaration be omitted in many commands where they don't need to initialize any data.

My worry here was that a large number of arguments would have be passed into the constructor, even if they weren't needed by the command. Your suggestion to put everything in the client resolves that.

Using this sounds a lot easier than having to constantly pass the same dependencies into functions over and over again. Every method that depends on the client needs to have it injected in the function.

In general, I like making it hard to pass around lots of dependencies easily because I think it encourages people to write cleaner code. My concern with having everything conveniently available via this is that instead of writing a good generateVibeEmbed() function with minimal dependencies, people would write something like my bad example sendMethodEmbed(). Writing a function like sendMethodEmbed() without a god object like interaction is much less likely to happen because you would have to manually pass all of the dependencies.

"Just use a partially applied function!"
You're going to go through that much effort just to avoid typing this? A class gives you this behavior for free!

Typing this isn't that bad, but I wouldn't call using a partially applied function a lot of effort either. Neither is "free".

Good question! If you wanna use classes and want a super reusable-stateless function, then why not create a top level function or a static function, and have your class reference it, like my example for generateVibeEmbed? Stateless utility functions are very common in OOP.

I wholeheartedly agree with this. My concern is that people would be less likely to do this if more things were available via Client or this.

Typescript doesn't let you compile this:

Good point. Typescript does defend against most errors of this kind. I admit this was also a bad example because properly using a free function fixes it.

registerUI would realistically be an instance method, not a variable, meaning there's more than one method in this class. Even better this can be assigned in Command, greatly reducing the amount of code in the example.

This is true.

Ok but what happens when I want multiple functions that use the client? For example, what if I want to add an enable and disable method:
I need shared state to keep track of that, if I'm left with an object, I can't expand much beyond it.
"Just pass it into the run()function!"

This is a good use case for this. It works with or without a class though; you can just put a disabled property on the command object and then set via this.disabled = true.
If complex enabling/disabling was a core feature I would be more inclined to go with a class. As it stands, I think a simple disabled boolean property is the most straightforward solution for disabling problematic commands at runtime.
I just have trouble coming up with a feature that truly needs to encapsulate some shared state between run() and another method.

So at what point does this ever-growing object being put into the run function become a god object?

My thought process is that using destructuring in the parameter list is akin to:
async run(interaction: Interaction, registerUI: SomeType, _: Storage) {}

with more convenient syntax. Where I normally would just have an unused argument like the storage in the above example, I can just not destructure that off.
There's no real "god object" in my opinion because it is constructed and then immediately torn apart.
If we instead attach all of the ever-growing dependencies to a client object or to the superclass, then I would consider either the client or the Command base class some degree of a god object.

Also here's some food for though, if I only destructure one of the many fields in the object given by run isn't that "hiding" dependencies? Because the other fields are still populated with values, you just aren't accessing them. This isn't reactjs where components sometimes only provide certain subset values in an object. In our case all of the values in object would be filled in all the time. So whether the dependency is hidden or not makes no difference. It's no different from the choice of accessing a field or not accessing a field. In terms of readability well that's subjective.

The difference to me is that after the destructuring, there is no reference to the unused properties anymore. The things you didn't destructure off aren't dependencies because there is no way to access them. If you instead use an approach with a container or client god object and no destructuring, you depend on the entire thing because any function or method down the line could access any part of the object.
That approach also makes it too easy to pass the god object along to other functions instead of just passing the minimal set of pieces that are actually needed. It enables antipatterns like the sendMethodEmbed() example.
I see it as similar to an import statement where you only import what you need. If you do import * as module from "module", then you have a dependency on the entire module. If you only import a single function though, even though the other properties are still populated with values, you don't have a dependency on them.
You have a point about the readability being subjective.

So I want to simply want to filter the things that have been either conceded or agreed on:

Free functions can apply to both a functional approach and a class-based approach.
Testing doesn't provide enough meaningful differences between both approaches.

Although the stubbing out of deps is up to debate.

this misuse is protected by typescript, and in most cases is an extremely rare issue.
Large constructors can be minimized by using putting properties in the client

I think the main disagreement(s) stem from the how one views destructuring vs fields.
A point was brought up in response to the idea that destructing is no different than class fields:

The difference to me is that after the destructuring, there is no reference to the unused properties anymore. The things you didn't destructure off aren't dependencies because there is no way to access them.

This is true, however, consider a class that inherits properties from its superclass. If you don't want to use the inherited properties, they don't have to be typed out at all in the subclass. This is analogous to destructuring, where you only pick the fields that you wanna use from a larger object.

If you instead use an approach with a container or client god object and no destructuring, you depend on the entire thing because any function or method down the line could access any part of the object.

Ideally only the client would be passed in, and yes other functions would depend on it, but thats intentional. client is an extremely useful object, thus it should be a hard dependency for Command's. Almost all discord.js objects have a reference to client, and I don't think it would be a bad idea for objects to follow suit.

So I want to simply want to filter the things that have been either conceded or agreed on:

Free functions can apply to both a functional approach and a class-based approach.
Testing doesn't provide enough meaningful differences between both approaches.
Although the stubbing out of deps is up to debate.
this misuse is protected by typescript, and in most cases is an extremely rare issue.
Large constructors can be minimized by using putting properties in the client

Yep, I agree on these points.

This is true, however, consider a class that inherits properties from its superclass. If you don't want to use the inherited properties, they don't have to be typed out at all in the subclass. This is analogous to destructuring, where you only pick the fields that you wanna use from a larger object.

I can agree partially with this. In my opinion, there's still the issue that the dependencies are now hidden in the superclass vs explicit in the function signature, but you're correct that neither approach requires any extra code for unused properties, which is good.

Ideally only the client would be passed in, and yes other functions would depend on it, but thats intentional. client is an extremely useful object, thus it should be a hard dependency for Command's. Almost all discord.js objects have a reference to client, and I don't think it would be a bad idea for our objects to follow suit.

So this is where I disagree. By attaching everything to client, client becomes a god object of sorts. The reason why this concerns me is that it makes it easier to write more tightly coupled functions. Consider the sendVibeEmbed() vs generateVibeEmbed() discussion above.
I think we agreed that generateVibeEmbed() was more reusable and less tightly coupled than sendVibeEmbed(), which made generateVibeEmbed() better. In my view, interaction serves as a god object in that case. If interaction was destructured into its constituent properties, it wouldn't have been possible to write sendVibeEmbed() because there wouldn't be an interaction to pass.
It is true that someone could just pass all of the properties into sendVibeEmbed() and use that poor design, but I think this is less likely since it would require passing a lot of arguments.
I worry a similar can of worms is opened with client or this having registerUI and storage attached. You're right that it is highly useful, because registerUI and storage are easily accessible from any method, but from my perspective, this easy access enables larger and more complicated functions that do too much. With destructuring and explicit passing of dependencies, writing "god functions" is less likely simply because manually passing the many dependencies is a pain.
This is a tradeoff, but I think it is a good one because it can guide command authors to write cleaner code.

So this is where I disagree. By attaching everything to client, client becomes a god object of sorts. The reason why this concerns me is that it makes it easier to write more tightly coupled functions. Consider the sendVibeEmbed() vs generateVibeEmbed() discussion above.

I think my comments about this above were a bit misleading. I wouldn't like stuffing a bunch of stuff in the client either. This is a violation of SRP. The way you work around this is by creating manager classes. If you take a look at the discord.js source, they use managers in a bunch of different places within their classes. This decouples the direct fields from a class while also keeping the accessibility. So if you don't want a Command to be directly responsible for UI related things, you can create a UIManager class and add a ui manager field to your Command class.

With destructuring and explicit passing of dependencies, writing "god functions" is less likely simply because manually passing the many dependencies is a pain.

Well that's the thing in the dispatch.ts, class. You'll have to provide all of the dependencies for the function in there. Because, you don't know what an implementing function will use, so you have provide every dependency upfront anyways.

So this is where I disagree. By attaching everything to client, client becomes a god object of sorts. The reason why this concerns me is that it makes it easier to write more tightly coupled functions. Consider the sendVibeEmbed() vs generateVibeEmbed() discussion above.

I think my comments about this above were a bit misleading. I wouldn't like stuffing a bunch of stuff in the client either. This is a violation of SRP. The way you work around this is by creating manager classes. If you take a look at the discord.js source, they use managers in a bunch of different places within their classes. This decouples the direct fields from a class while also keeping the accessibility. So if you don't want a Command to be directly responsible for UI related things, you can create a UIManager class and add a ui manager field to your Command class.

This can sometimes help (though I personally don't really enjoy manager classes), but I don't think this really fixes the problem that any method can easily access any dependency on the superclass. The way I see this having a negative impact is that people will write bigger and less maintainable/reusable methods.
Say someone wants to fetch some data from persistent storage and then generate some kind of UI from that data. With things available via this.storage or the like, people might write something like this.generateLeaderboardUIFromStorage() as opposed to using a few smaller free functions like fetchLeaderboardData(storage) and generateLeaderboardUI(data, registerUI). I find that if I'm forced to do something like generateLeaderboardFromStorage(storage, data, registerUI), I'm much more likely to realize I'm doing too much in a single function.

With destructuring and explicit passing of dependencies, writing "god functions" is less likely simply because manually passing the many dependencies is a pain.

Well that's the thing in the dispatch.ts, class. You'll have to provide all of the dependencies for the function in there. Because, you don't know what an implementing function will use, so you have provide every dependency upfront anyways.

Right, but this is no worse than injecting everything into the Command superclass. Ultimately if we want to have polymorphism over commands and treat them all the same, we'll always suffer this issue in one way or another.

This can sometimes help (though I personally don't really enjoy manager classes), but I don't think this really fixes the problem that any method can easily access any dependency on the superclass. The way I see this having a negative impact is that people will write bigger and less maintainable/reusable methods.

The idea of breaking functions down, is the shared between OOP and FP. The only time people write functions that combine other functions is when they have a private helper. Regardless if someone added a large function that needlessly combined other free functions I probably wouldn't accept it. It's just bad design in general.

Say someone wants to fetch some data from persistent storage and then generate some kind of UI from that data. With things available via this.storage or the like, people might write something like this.generateLeaderboardUIFromStorage() as opposed to using a few smaller free functions like fetchLeaderboardData(storage) and generateLeaderboardUI(data, registerUI). I find that if I'm forced to do something like generateLeaderboardFromStorage(storage, data, registerUI), I'm much more likely to realize I'm doing too much in a single function.

I don't think most people would do that and if they did it would be a private helper.
Two of the indicators of a functions that does too much is:

Long Function names
logic within the function body that used in other functions.

IMO, this is pretty easy spot.

This can sometimes help (though I personally don't really enjoy manager classes), but I don't think this really fixes the problem that any method can easily access any dependency on the superclass. The way I see this having a negative impact is that people will write bigger and less maintainable/reusable methods.

The idea of breaking functions down is shared between OOP and FP. The only time people write functions that combine other functions is when they have a private helper. Regardless if someone added a large function that needlessly combined other free functions I probably wouldn't accept it. It's just bad design in general.

Say someone wants to fetch some data from persistent storage and then generate some kind of UI from that data. With things available via this.storage or the like, people might write something like this.generateLeaderboardUIFromStorage() as opposed to using a few smaller free functions like fetchLeaderboardData(storage) and generateLeaderboardUI(data, registerUI). I find that if I'm forced to do something like generateLeaderboardFromStorage(storage, data, registerUI), I'm much more likely to realize I'm doing too much in a single function.

I don't think most people would do that and if they did it would be a private helper.
Two of the indicators of a functions that does too much is:
* Long Function names

* logic within the function body that used in other functions.

IMO, this is pretty easy spot.

Sure, this isn't hard for us to identify as bad because we're intimately familiar with potential bad idioms, but for less experienced developers, it is tempting to attach functions that should be free functions to the class and to use this.xyz instead of a parameter. We can catch it in review of course, but I suspect we'll get better quality code if we don't put things on this. This antipattern can happen with my preferred style as well in the form of closures defined in run(), but people don't seem to do this nearly as much as creating helper methods that should be free functions.
If you can come up with a compelling need for a second method needed across all the commands that isn't better written as an injected free function (ie a dependency fundamental to all commands), I'm willing to concede that a class is better. I just don't think that need currently exists.

I see our Commands as a collection of read only declarative fields with a single highly polymorphic function, run(), that executes the behavior needed when the command is invoked. I don't feel like there is any shared state or other fundamental method that exists, which is what leads me toward a classless design.

Sure, this isn't hard for us to identify as bad because we're intimately familiar with potential bad idioms, but for less experienced developers, it is tempting to attach functions that should be free functions to the class and to use this.xyz instead of a parameter. We can catch it in review of course, but I suspect we'll get better quality code if we don't put things on this.

Most people are coming from using OOP. Especially, Armin and Vijay who worked on Lion. Well and me too. There's nothing wrong with creating your own instance methods if they're only used in one place. The reason the generateVibeEmbed looked bad was because it was trying to be shared from a class, instead of it being a static function.

This antipattern can happen with my preferred style as well in the form of closures defined in run(), but people don't seem to do this nearly as much as creating helper methods that should be free functions.

IMO, This feels like an over-generalization.
I can accept the FP pitfalls just like I can accept the pitfalls of OOP, I don't think either of these are more or less avoidable than the other.

If you can come up with a compelling case for a second method needed across all the commands that isn't better written as an injected free function (ie a dependency fundamental to all commands), I'm willing to concede that a class is better. I just don't think that need currently exists.

I believe it's not about how many methods a class has but instead what properties a class can easily use. If I send a storage and registerUI() to only the run() function, I then have to add a another parameter to the other functions helper function for a specific command I’m building. This is especially true for persistent storage. This can be seen as annoying and somewhat tedious. It’s not very consumer friendly. You can argue that we can see our dependencies more explicitly, but it’s not as pragmatic for the end user. I once again return to my reactjs example. React provides the context api and it’s heavily used. We can pass everything down via the props (which is very explicit) but that scales really badly. We can strive for a codebase that’s really nice to maintain on our end, but create with something that annoys the end developers.

Sure, this isn't hard for us to identify as bad because we're intimately familiar with potential bad idioms, but for less experienced developers, it is tempting to attach functions that should be free functions to the class and to use this.xyz instead of a parameter. We can catch it in review of course, but I suspect we'll get better quality code if we don't put things on this.

Most people are coming from using OOP. Especially, Armin and Vijay who worked on Lion. Well and me too. There's nothing wrong with creating your own instance methods if they're only used in one place. The reason the generateVibeEmbed looked bad was because it was trying to be shared from a class, instead of it being a static function.

There isn't anything "wrong" with using an instance method. My thought is that just because your instance method is "only used in one place" for now doesn't mean that it won't be useful elsewhere down the line. generateVibeEmbed is only used in one place currently, but by making it a free function, it is easier to use it elsewhere later. Why not go for the flexibility and composability of a free function by default? (and consider instance methods if you find yourself passing the same parameters over and over to a set of functions)

This antipattern can happen with my preferred style as well in the form of closures defined in run(), but people don't seem to do this nearly as much as creating helper methods that should be free functions.

IMO, This feels like an over-generalization.
I can accept the FP pitfalls just like I can accept the pitfalls of OOP, I don't think either of these are more or less avoidable than the other.

Fair enough, this is just in my experience with code I've seen.

If you can come up with a compelling case for a second method needed across all the commands that isn't better written as an injected free function (ie a dependency fundamental to all commands), I'm willing to concede that a class is better. I just don't think that need currently exists.

I believe it's not about how many methods a class has but instead what properties a class can easily use. If I send a storage and registerUI() to only the run() function, I then have to add a another parameter to the other functions helper function for a specific command I’m building. This is especially true for persistent storage. This can be seen as annoying and somewhat tedious. It’s not very consumer friendly. You can argue that we can see our dependencies more explicitly, but it’s not as pragmatic for the end user. I once again return to my reactjs example. React provides the context api and it’s heavily used. We can pass everything down via the props (which is very explicit) but that scales really badly. We can strive for a codebase that’s really nice to maintain on our end, but create something that annoys the end developers.

I fear we're stretching the React analogy a bit far, but I actually find this to often be a better alternative than context. The general idea, more or less, is that if passing arguments around starts to become tedious, then you might be passing the wrong argument.
A more concrete example might be that if you're annoyed by passing storage around into multiple functions, then maybe you should instead read from storage at the top level in run() and pass the read data into your functions instead.
After a bit of reflection, registerUI is a similar case. You probably don't need to pass it outside of run() in the vast majority of cases; you can just generate your UI in a free function if you need to and then always call registerUI in run(). There's no need to push that responsibility outside of run().

I think you can make a master's thesis out of all this.

I fear we're stretching the React analogy a bit far, but I actually find this to often be a better alternative than context. The general idea, more or less, is that if passing arguments around starts to become tedious, then you might be passing the wrong argument.

It's fine to have that opinion but the point I'm trying to make is, it seems like many people even the ones making wildly popular react libraries would disagree. There's a fine line between making something easy to maintain, and it being easy to use.  Sometimes you can compose to avoid the context api both in a lot of other cases you can't. You can't predict how many levels deep you component trees go. If you just follow the idea of using composition as apposed to the context, then you'll end up doing more refactoring later. If using composition works in your specific situation then that's awesome, use that instead, but you also need to consider the cases where it potentially won't.

A more concrete example might be that if you're annoyed by passing storage around into multiple functions, then maybe you should instead read from storage at the top level in run() and pass the read data into your functions instead. Hence why it's being used so heavily. People don't want to deal with manually managing dependencies sometimes, and that's ok.

If we can avoid putting more work/structural thought on the end developer I would prefer that. We're making an api to simplify development, the intention is to not introduce more complexity. With this we're expecting them to do more housekeeping.

After a bit of reflection, registerUI is a similar case. You probably don't need to pass it outside of run() in the vast majority of cases; you can just generate your UI in a free function if you need to and then always call registerUI in run(). There's no need to push that responsibility outside of run().

I disagree, what if I have a helper function that generates the message options to be sent out? It's not that uncommon. With classes I don't need to inject anything into my function, less for me to worry about as an end dev. With the FP we need to inject it. And we may need to do it multiple times. That's more work, and a worse end developer experience.
I think we should be building API's to be easy to use in a vast amount of circumstances. Even if it doesn't match our circumstances now. I feel like saying assuming that a user "probably won't" do something isn't the best way to approach API's meant to work with many different situations. Especially if you have a built-in and familiar option that can easily support these needs.

I fear we're stretching the React analogy a bit far, but I actually find this to often be a better alternative than context. The general idea, more or less, is that if passing arguments around starts to become tedious, then you might be passing the wrong argument.

It's fine to have that opinion but the point I'm trying to make is, it seems like many people even the ones making wildly popular react libraries would disagree. There's a fine line between making something easy to maintain, and it being easy to use. Sometimes you can compose to avoid the context api both in a lot of other cases you can't. You can't predict how many levels deep you component trees go. If you just follow the idea of using composition as apposed to the context, then you'll end up doing more refactoring later. If using composition works in your specific situation then that's awesome, use that instead, but you also need to consider the cases where it potentially won't.

You make a good point about cases where the existing system doesn't work. I guess I would just like to "wait and see" as more commands are added so we have a better idea of what issues we are actually running into that aren't solved well by the current system of injecting things through run(). If you're willing, I'd like to try my approach and see if we run into problems, and if we do, we can go for a comprehensive refactor with classes with a clear problem to solve. I tend to believe we won't really run into cases where it won't work.

A more concrete example might be that if you're annoyed by passing storage around into multiple functions, then maybe you should instead read from storage at the top level in run() and pass the read data into your functions instead. Hence why it's being used so heavily. People don't want to deal with manually managing dependencies sometimes, and that's ok.

If we can avoid putting more work/structural thought on the end developer I would prefer that. We're making an api to simplify development, the intention is to not introduce more complexity. With this we're expecting them to do more housekeeping.

I'm all for reducing work on the part of end developers, but not at the expense of high quality code. If and when we start to see patterns emerge in housekeeping issues, then I'm entirely willing to change the entire architecture if necessary.

After a bit of reflection, registerUI is a similar case. You probably don't need to pass it outside of run() in the vast majority of cases; you can just generate your UI in a free function if you need to and then always call registerUI in run(). There's no need to push that responsibility outside of run().

I disagree, what if I have a helper function that generates the message options to be sent out? It's not that uncommon. With classes I don't need to inject anything into my function, less for me to worry about as an end dev. With the FP we need to inject it. And we may need to do it multiple times. That's more work, and a worse end developer experience.

There shouldn't be a reason we can't write a function to generate the message options to be sent out; I think an example or clarification here would help me better understand your concern.

I think we should be building API's to be easy to use in a vast amount of circumstances. Even if it doesn't match our circumstances now. I feel like saying assuming that a user "probably won't" do something isn't the best way to approach API's meant to work with many different situations. Especially if you have a built-in and familiar option that can easily support these needs.

I agree on making APIs easy to use. I don't agree as much on the "even if it doesn't match our circumstances now". It is difficult to impossible to predict what future constraints we will want. I firmly believe the only thing you can really do is write code to be as flexible as possible now and then constantly iterate as new needs arise. It is better to allow some temporary duplication of code than to choose the wrong abstraction.

Ok I've decided that we'll use @rob-3's method of passing data in and we'll take it from there. And we'll use a "wait and see" approach to this. If we run into issues we can look into classes.

Don't merge this or bother reviewing it yet. After our discussion I have a few ideas about how it could be improved.

I'm not planning on merging anything yet, but you can merge whenever you're satisfied with your implementation.

Closed in favor of newer PR


Cannot use a result of 'MemoryMarshal.CreateSpan(ref char, int)' in this context because it may expose variables referenced by parameter 'reference' outside of their declaration scope

Actual behavior
Compilation fails when using C# preview and msbuild.
  CompilerServer: server failed - server rejected the request due to analyzer / generator issues 'analyzer assembly 'C:\Users\virzak\.nuget\packages\microsoft.windows.cswin32\0.2.10-beta\analyzers\cs\System.Runtime.CompilerServices.Unsafe.dll' has MVID 'd22a1fa6-8134-4c26-bb36-e67aceee55b8' but loaded assembly 'System.Runtime.CompilerServices.Unsafe, Version=<IP_ADDRESS>, Culture=neutral, PublicKeyToken=b03f5f7f11d50a3a' has MVID 'bc84029b-1123-4266-bc37-9616e5f03999'' - 3ab8afc4-73d7-4553-b014-16f9a787539b
Q:\Code\Sandbox\SetupDiGetDeviceInterfaceDetail\Microsoft.Windows.CsWin32\Microsoft.Windows.CsWin32.SourceGenerator\Windows.Win32.SP_DEVICE_INTERFACE_DETAIL_DATA_W.g.cs(47,66): error CS8170: Struct members cannot return 'this' or other instance members by reference [Q:\Code\Sandbox\SetupDiGetDeviceInterfaceDetail\SetupDiGetDeviceInterfaceDetail.csproj]
Q:\Code\Sandbox\SetupDiGetDeviceInterfaceDetail\Microsoft.Windows.CsWin32\Microsoft.Windows.CsWin32.SourceGenerator\Windows.Win32.SP_DEVICE_INTERFACE_DETAIL_DATA_W.g.cs(47,37): error CS8347: Cannot use a result of 'MemoryMarshal.CreateSpan<char>(ref char, int)' in this context because it may expose variables referenced by parameter 'reference' outside of their declaration scope [Q:\Code\Sandbox\SetupDiGetDeviceInterfaceDetail\SetupDiGetDeviceInterfaceDetail.csproj]
Done Building Project "Q:\Code\Sandbox\SetupDiGetDeviceInterfaceDetail\SetupDiGetDeviceInterfaceDetail.csproj" (default targets) -- FAILED.

Expected behavior
A clear and concise description of what you expected to happen.
Repro steps

NativeMethods.txt content:

SetupDiGetDeviceInterfaceDetail

Run the following to reproduce:
dotnet new classlib -o SetupDiGetDeviceInterfaceDetail --langVersion preview
dotnet add SetupDiGetDeviceInterfaceDetail package Microsoft.Windows.CsWin32 --version 0.2.10-beta
echo "SetupDiGetDeviceInterfaceDetail" > SetupDiGetDeviceInterfaceDetail/NativeMethods.txt
msbuild .\SetupDiGetDeviceInterfaceDetail\ /p:Runtimeidentifier=win-x64 /restore

Any of your own code that should be shared?

Context

CsWin32 version: 0.2.10-beta
Target Framework: net6.0, net7.0
LangVersion (if explicitly set by project): 11 preview

Either removing preview flag, or compiling with dotnet doesn't produce this issue

Well, that's disappointing but also not entirely a surprise. We knew (as the code comment on the failing code indicates) that this was a risky method. I guess we'll have to find another way to make inline spans easy to use. I thought I'd heard that C# 11 was supposed to make fixed arrays better, but I can't anything about it here.

The fix for this will be to adjust the emitted code to attribute the AsSpan() method with [UnscopedRef]. This attribute tells the compiler about how the ref escapes the current method, and in fact will allow us to remove the warning on the doc comment because at this point the compiler will guard the caller's code for us. When targeting < .NET 7 (or more precisely, when UnscopedRefAttribute isn't available), we can emit that attribute as well. The attribute should always be emitted as internal.
While these changes need only be made for C# 11+ compilations, as none of these are required or helpful down-level, it is conceivable that a C# 11 project that references the generated APIs may exist, so the attribute on the member may still be useful to apply.

Looks like this problem just went away after installing 7.0.100-rc.1.22431.12. Version 0.2.10-beta all of a sudden works.

Good. Yes, the fix I made to the source generator was "forward facing" to an SDK update that would carry a compiler change.


Conflict between --saucelabs and --browser options

On rna unit command there is a conflict between --saucelabs option and --browser option. It gives en error about not finding CHROME_CI as registered browser.

Will be released in the upcoming RNA 3.