Skip to content

Conversation

@samkim-crypto
Copy link

@samkim-crypto samkim-crypto commented Mar 24, 2024

Problem

With clap-v3, the value_of, value_of_t, and other value_of_... functions have been deprecated. These functions were convenient in that they performed a type conversion on the parsed string argument. The function that replaces these functions get_one::<T>(...) can also perform type conversion by specifying a generic type T; unfortunately, the need to specify T as a concrete type makes usage less flexible.

For example, consider the word count argument.

pub fn word_count_arg<'a>() -> Arg<'a> {
    Arg::new(WORD_COUNT_ARG.name)
        .long(WORD_COUNT_ARG.long)
        .value_parser(PossibleValuesParser::new(["12", "15", "18", "21", "24"]))
        .default_value("12")
        .value_name("NUMBER")
        .takes_value(true)
        .help(WORD_COUNT_ARG.help)
}

With the possible values specified as strings, calling get_one::<usize>(WORD_COUNT_ARG.name) will produce a downcast error since we have not specified how to convert each string to usize.

The canonical way to take care of this in clap-v3 is to create an enum WordCount { ... }, implement Into<usize> and call let var: usize = get_one::<WordCount>(...).into().

Alternatively, we can let caller take care of the string conversion get_one::<String>(...).map(|word_count| word_count.parse::<usize>()).

Both methods seem to produce unnecessary and inelegant code.

Summary of Changes

Add a function try_get_word_count that can directly parse usize from arg matches.

Fixes #

@codecov-commenter
Copy link

codecov-commenter commented Mar 24, 2024

Codecov Report

Attention: Patch coverage is 0% with 13 lines in your changes are missing coverage. Please review.

Project coverage is 81.9%. Comparing base (b6d2237) to head (e42db22).
Report is 5 commits behind head on master.

Additional details and impacted files
@@           Coverage Diff           @@
##           master     #411   +/-   ##
=======================================
  Coverage    81.9%    81.9%           
=======================================
  Files         840      840           
  Lines      228058   228070   +12     
=======================================
+ Hits       186795   186820   +25     
+ Misses      41263    41250   -13     

@samkim-crypto samkim-crypto marked this pull request as ready for review March 24, 2024 02:53
@samkim-crypto samkim-crypto force-pushed the clap-v3-utils/acquire-wordcount branch from 0f8960f to e42db22 Compare March 25, 2024 23:53
@samkim-crypto samkim-crypto changed the title [clap-v3-utils] Add acquire_word_count [clap-v3-utils] Add try_get_word_count Mar 26, 2024
@samkim-crypto samkim-crypto merged commit f6a3608 into anza-xyz:master Mar 26, 2024
OliverNChalk pushed a commit to OliverNChalk/agave that referenced this pull request Nov 11, 2025
This change includes all of the following changes as well:

---

Author: kirill lykov <[email protected]>
Date:   Tue May 28 20:20:39 2024 +0200

Add transaction bench client (anza-xyz#339)

New client application that sends transactions using TPU protocol.

First, this client creates accounts necessary for the transaction
generation.

Second, it generates transactions executing specified program and sends
them to the upcoming leaders.

Contrary to other clients, it doesn't use the `TpuClient` and the
`ConnectionCache` to interact with validators, but uses an alternative
client network layer implementation.

---

Author: Alex Peng <[email protected]>
Date:   Tue Jun 4 02:31:16 2024 -0500

transaction-bench: fix underflow in ensure_authority_balance (anza-xyz#341)

---

Author: kirill lykov <[email protected]>
Date:   Fri Jul 26 10:02:39 2024 +0200

add 3 tests for client network component (anza-xyz#365)

This PR is the first in a series of PR introducing integration tests for
client network component.
It introduces the following changes:

* add first basic integration test

* add test_connection_denied_until_allowed

* add test for connection pruning

* retry connecting when pruned

* Introduce IoErrorWithPartialEq to check that QuicError is as expected

---

Author: kirill lykov <[email protected]>
Date:   Mon Aug 5 18:30:09 2024 +0200

Handle error with connection properly (anza-xyz#383)

When one of the connections fails to open, we stop the whole client.
It doesn't work on testnet because there most of the nodes drop TPU
traffic despite of saying through gossip that it is allowed (need to
ping foundation on that).

---

Author: kirill lykov <[email protected]>
Date:   Thu Aug 8 22:25:54 2024 +0200

Read/write for AccountsFile to use on testnet (anza-xyz#381)

Only the `transaction-bench/src/network/` and `transaction-bench/tests`
parts of this change was merge in.

Illia Bobyr: Migrated to quinn 0.11 and refactored the tests a bit.

---

Author: kirill lykov <[email protected]>
Date:   Sat Aug 17 15:10:19 2024 +0200

Handle instruction error in client-test-program (anza-xyz#411)

Due to upstream changes errors are not propagated properly anymore when
calling try_from_slice. This PR fixes the problem exactly the same way
as in anza-xyz#387

---

Author: kirill lykov <[email protected]>
Date:   Tue Aug 20 10:01:36 2024 +0200

Add structure to collect statistics about send txs erros (anza-xyz#367)

There is no way to check why certain packets have not been delivered.
By propagating this information to the caller, we can use it not only in
the client code but also in tests.
This change Introduces a structure which accumulates relevant counters
per node IP and returns this information to the client code.

Illia Bobyr: Migrated to quinn 0.11 and refactored a bit.

Refactored-by: Illia Bobyr <[email protected]>

---

Author: kirill lykov <[email protected]>
Date:   Mon Aug 26 18:37:07 2024 +0200

Add rate limiting test to client (anza-xyz#368)

This PR introduces integration test to check that when rate-limiting has
happen we increment correct counters on client side.

---

Author: kirill lykov <[email protected]>
Date:   Tue Aug 27 16:34:57 2024 +0200

Improve stream sending (anza-xyz#372)

* stop using multistream connection

* add throttling test

* remove stream's finish call. It will be called when stream is dropped
  anyways, but if we call finish explicitly we will wait for ACK, so the
  behavior will be like if we had RW=1 tx. This change makes a x1000
  difference on a connection with high latency.

Illia Bobyr: Migrated to quinn 0.11, added cancellation in order to
support clean shutdown, and refactored the tests a bit.

Refactored-by: Illia Bobyr <[email protected]>

---

Author: kirill lykov <[email protected]>
Date:   Thu Sep 5 18:54:01 2024 +0200

use testing_utilities (anza-xyz#427)

We use modified copy-pasted code from streamer to setup server/client in
integration tests for network component.

Later original code was updated to be reusable.

This PR removes old copy-pasted code and uses modified streamer's
testing_utilities code.

---

Author: kirill lykov <[email protected]>
Date:   Tue Sep 10 12:06:08 2024 +0200

Move leader updater to network and add documentation (anza-xyz#426)

---

Author: kirill lykov <[email protected]>
Date:   Tue Sep 10 16:30:34 2024 +0200

Add documentaiton to network code (anza-xyz#425)

---

Author: kirill lykov <[email protected]>
Date:   Tue Sep 10 20:58:04 2024 +0200

Don't ignore `test_no_host` (anza-xyz#435)

---

Author: kirill lykov <[email protected]>
Date:   Wed Sep 11 20:34:09 2024 +0200

Address Illia's comments (anza-xyz#438)
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Labels

None yet

Projects

None yet

Development

Successfully merging this pull request may close these issues.

3 participants