GH 731 review #341

czarte · 2023-09-15T10:28:28Z

No description provided.

…roperly

…d bools for data_dir, config_file and real_user to MultiConfig, fixing the test

…e able to push into Vcl Vector, add processing in MultiConfig, add processing in server_initializer_collected_params

… to MultiConfig and selecting them to computed_value_names on behalf of their type

…ix config file arguments data

…inery to determine if real-user is specified in config-file

…r_specific_data

…zer_collected_params, closure for keeping in node_configurator, final removal of redundant construction of multiconfig

masq_lib/src/multi_config.rs

dnwiebe · 2023-09-15T10:55:48Z

masq_lib/src/multi_config.rs

+                vcl.vcl_args().iter().for_each(|vcl_arg| {
+                    match vcl.is_computed() {
+                        true => computed_value_names.insert(vcl_arg.name().to_string()),
+                        false => computed_value_names.remove(&vcl_arg.name().to_string())


I can't say, with the knowledge in my head, whether removing this value from the computed_value_names is correct. Can you come up with a few sensible, realistic tests involving data that can be both specified and computed, and run it through this code to see if the result makes sense?

I'm not suggesting that this code doesn't work; I'm unsure whether the situation it creates is the one we want.

Example: if the user specifies the data-directory without a chain, do we add a computed value with the chain appended? If so, shouldn't that computed value win over the specified value? (I don't know.)

dnwiebe · 2023-09-15T10:57:12Z

masq_lib/src/multi_config.rs

+                })
+            });
+        }
+        let merged: Box<dyn VirtualCommandLine> = vcls.into_iter().fold(initial, |so_far, vcl | -> Box<dyn VirtualCommandLine> {


You might not need to specify the return type for the closure. If you do, go ahead, but it seems to me that the definition of fold() should take care of that for you.

dnwiebe · 2023-09-15T10:57:56Z

masq_lib/src/multi_config.rs

@@ -61,24 +62,41 @@ impl<'a> MultiConfig<'a> {
        schema: &App<'a, 'a>,
        vcls: Vec<Box<dyn VirtualCommandLine>>,
    ) -> Result<MultiConfig<'a>, ConfiguratorError> {
+        let mut computed_value_names = HashSet::new();
        let initial: Box<dyn VirtualCommandLine> =


Could you move this down so that it's next to where it's used? It'd be easier to read.

dnwiebe · 2023-09-15T11:11:54Z

masq_lib/src/multi_config.rs

@@ -139,6 +157,13 @@ impl<'a> MultiConfig<'a> {
        ConfiguratorError::required("<unknown>", &format!("Unfamiliar message: {}", e.message))
    }

+    pub fn is_user_specified(&self, value_name: &str) -> bool {


In the old code, there's a macro that looks like this:

#[macro_export] macro_rules! value_user_specified_m { ($m:ident, $v:expr, $t:ty) => {{ let user_specified = $m.occurrences_of($v) > 0; let matches = $m.arg_matches_ref(); match value_t!(matches, $v, $t) { Ok(v) => (Some(v), user_specified), Err(_) => (None, user_specified), } }}; }

That seems incompatible with this code. Do the two pass the same tests?

Further observation: The macro above does not seem to be used anywhere in the codebase anymore. (Check to make sure I'm right about this.) If it's not, there's no need to keep it around, and once it's deleted its meaning can be redefined as desired.

value_user_specified_m! macro is definitely not comparable to is_user_specified method in MultiConfig. value_user_specified_m! macro is returning true even if the value is computed.

dnwiebe · 2023-09-15T11:42:49Z

masq_lib/src/multi_config.rs

@@ -360,6 +432,16 @@ impl VirtualCommandLine for ConfigFileVcl {
        vcl_args_to_args(&self.vcl_args)
    }
 }
+/*


Commented-out code

node/src/daemon/setup_reporter.rs

node/src/node_configurator/mod.rs

dnwiebe · 2023-09-15T11:51:07Z

node/src/node_configurator/mod.rs

-        .arg(real_user_arg())
-        .arg(data_directory_arg(DATA_DIRECTORY_HELP))
-        .arg(config_file_arg());
+) -> Result<(PathBuf, bool, PathBuf, bool, RealUser, bool, Box<dyn VirtualCommandLine>), ConfiguratorError> {


This tuple is getting awfully long, don't you think?

bertllll

Check the functions create_preorientation_args and server_initializer_collected_params, seen just partly, and the two first tests.

bertllll · 2023-09-15T20:49:09Z

node/src/node_configurator/mod.rs

-        .arg(config_file_arg());
+) -> Result<(PathBuf, bool, PathBuf, bool, RealUser, bool, Box<dyn VirtualCommandLine>), ConfiguratorError> {
+    let env_args = Box::new(EnvironmentVcl::new(&app)).args();
+    let args_to_vec = args.to_vec();


This name just confuses people, especially because it is not an important piece of information that this particular method was used to form the data. Instead call it according to the state it got from the look at it afterwards, like: vec_of_args.

bertllll · 2023-09-15T20:59:18Z

node/src/node_configurator/mod.rs

-        .arg(data_directory_arg(DATA_DIRECTORY_HELP))
-        .arg(config_file_arg());
+) -> Result<(PathBuf, bool, PathBuf, bool, RealUser, bool, Box<dyn VirtualCommandLine>), ConfiguratorError> {
+    let env_args = Box::new(EnvironmentVcl::new(&app)).args();


I would like more if you built this variable first inside the function you call "create_preorientation_args". It doesn't need to come in from outside. Especially not, if you already supply the "app" argument to the same fn, which you do need to get the env_args.

bertllll · 2023-09-15T21:00:58Z

node/src/node_configurator/mod.rs

+) -> Result<(PathBuf, bool, PathBuf, bool, RealUser, bool, Box<dyn VirtualCommandLine>), ConfiguratorError> {
+    let env_args = Box::new(EnvironmentVcl::new(&app)).args();
+    let args_to_vec = args.to_vec();
+    let pre_orientation_args = match create_preorientation_args(env_args, args_to_vec.clone(), &app) {


It feels fishy that you need to clone things that often. Do always your best to use only a slice of Vec instead of the whole Vec. ... create_preorientation_args(env_args, &args_to_vec, &app)...

bertllll · 2023-09-15T21:12:41Z

node/src/node_configurator/mod.rs

+    }
+}
+
+fn create_preorientation_args(envargs: Vec<String>, argstovec: Vec<String>, app: &App) -> Result<Box<dyn VirtualCommandLine>, ConfigFileVclError> {


Does this really need to say "preorientation" which is a word so vague from just a look at? I think it would help much better if you mentioned somewhat that this will serve to determine the location of the config file.

Also, I realized that you've adopted the naming style with the word orientation ("orientation_schema") from the original code but you might not understand enough why it was chosen. I don't know the full story either but it probably refers to "getting oriented about where the config file lies", I'm not sure if preorientation wouldn't drift too away from what idea had borne. Please think about keeping the code clear for the future devs and in order to do that never be shy to express the functionality of the function with more meaningful words.

bertllll · 2023-09-15T21:14:30Z

node/src/node_configurator/mod.rs

+    let (env_config_file, env_data_dir, env_real_user) = config_file_real_user_data_dir_from_enumerate(envargs);
+    let (cmd_config_file, cmd_data_dir, cmd_real_user) = config_file_real_user_data_dir_from_enumerate(argstovec);
+    let mut combined_vcl: CombinedVcl = CombinedVcl { content: vec![] };
+    let combine_vcl = | name: String, vcl: &mut CombinedVcl, cmd_str: &String, env_str: &String | {


This would pollute the space here less if it is a separate standalone function. Notice it captures no outer variable so we don't need a closure here, a classical function would do well.

bertllll · 2023-09-15T22:45:00Z

node/src/node_configurator/node_configurator_standard.rs

+                )
+            }
+            false => {
+                println!("real-user is not user specified in Environment");


Again, a legitimate failure. Use a panic and change other things so that the test doesn't behave unanticipatedly.

bertllll · 2023-09-15T22:47:01Z

node/src/node_configurator/node_configurator_standard.rs

+        #[cfg(not(target_os = "windows"))]
+        match vcl_multiconfig.is_user_specified("--real-user") {
+            true => {
+                match env_multiconfig.is_user_specified("--real-user") {


What?? You allow two different paths and you claim both be right. This is far from a firm test. Again, EnvironmentGuard should help you out.

bertllll · 2023-09-15T22:50:43Z

node/src/node_configurator/node_configurator_standard.rs

+                }
+            }
+            false => {
+                println!("real-user is not user specified in Command-line");


Hmm, this should be undoubtly a panic.

Okay, let's rephrase what you should do. You need to get rid of the sometimes-present factor at your parameters and so the flags for ENV_DATA_DIR and CMD_DATA_DIR.

There must be a solid path thorough the test without multiple possibilities!!

bertllll · 2023-09-15T22:51:18Z

node/src/node_configurator/node_configurator_standard.rs

+                ()
+            }
+        }
+        println!("env_data_directory {:#?}", env_data_directory);


Left over debug fn.

bertllll · 2023-09-15T22:52:55Z

node/src/node_configurator/node_configurator_standard.rs

+        println!("env_data_directory {:#?}", env_data_directory);
+        match vcl_multiconfig.is_user_specified("--data-directory") {
+            true => {
+                match env_data_directory && !cmd_data_directory {


I would repeat myself here. This whole branching structure need to be rewritten to work without conditional truths.

bertllll · 2023-09-17T18:12:40Z

node/src/node_configurator/mod.rs

+    let mut arg_match = None;
+    for (i, arg) in configs.iter().enumerate() {
+        if arg.as_str() == needle {
+            arg_match = Some(configs[i + 1].to_string())


How can you be sure that every parameter has a value or not more than one. (There is a big set of various args among which the ones you are searching here are just a small part. So I believe the problem is realistic.) If that goes true then your algorithm will hopelessly fail. You could easily end up remembering a value that would be actually a parameter name or for example a second value. I think it creates space for later developed errors.

On the other hand, it is possible that you are confident this does not threaten. I'm okay if you just think a couple of mins about this.

bertllll · 2023-09-17T18:15:49Z

node/src/node_configurator/mod.rs

+    (config_match, data_dir_match, real_user_match)
+}
+
+fn argument_from_enumerate(configs: Vec<String>, needle: String) -> Option<String> {


The use of the enumerate() fn has nothing to do with the purpose of this traversing algorithm. So there is no need to mention it in the name of this function. What about something similar to "find_optional_arg_and_return_value" or "return_arg_value_from_set_if_present"

bertllll · 2023-09-17T18:24:59Z

node/src/node_configurator/mod.rs

+    arg_match
+}
+
+pub fn determine_user_specific_data (


From my view, all the input is user specific data, because its given by a certain unique person.
If not like that, I think I understand what you hand in mind.
What about "determine_users_data_sources_and_os_identity" or "determine_input_location_and_user".

bertllll · 2023-09-17T18:29:57Z

node/src/node_configurator/mod.rs

-    let data_directory = match data_directory_path {
-        Some(data_dir) => data_dir,
-        None => data_directory_from_context(dirs_wrapper, &real_user, chain),
+    let (config_file, data_dir, mut real_user) = config_file_real_user_data_dir_from_enumerate(orientation_vcl.args());


Again, the word "enumerate" tells the reader nothing about the purpose of this function. It might be saying something to you as the author, but other people don't care.

Maybe you could drop "_from_enumerate".

bertllll · 2023-09-17T18:34:15Z

node/src/node_configurator/mod.rs

-        Some(data_dir) => data_dir,
-        None => data_directory_from_context(dirs_wrapper, &real_user, chain),
+    let (config_file, data_dir, mut real_user) = config_file_real_user_data_dir_from_enumerate(orientation_vcl.args());
+    let config_user_specified = !config_file.is_empty();


Here we have a good example of why the version with the treatment of these as optional (With the use of Option) would look neater. It would probably be config_file_opt.is_some() whilst if one sees config_file.is_empty(), the first thing they would think of is that the file contains no data. Which is quite far from what we want. I do think you should use the Option type for your strings as it is most idiomatic in Rust.

bertllll · 2023-09-17T19:42:02Z

node/src/node_configurator/node_configurator_standard.rs

+        real_user,
+        real_user_specified,
+        pre_orientation_args) = determine_user_specific_data(dirs_wrapper, &app, args)?;
+    let mut full_multi_config_vec: Vec<Box<dyn VirtualCommandLine>> = vec![


This variable's name is confusing. Look where it goes in the end, this siunds as if this was already the final product. The truth is this is only a feed of the constructor of a multi config. So I'd expect something like compete_multi_config_args.

I was thinking that giving it the name "full" or "complete" might not be so appropriate because you still need to "fill it with more parameters" and only after it we can call it really "complete". But that's a detail.

bertllll · 2023-09-17T19:44:45Z

node/src/node_configurator/node_configurator_standard.rs

-        Err(e) => return Err(ConfiguratorError::required("config-file", &e.to_string())),
+    let mut fill_specified_or_unspecified_box = |key: String, value: String, specified: bool | {
+        match specified {
+            true => full_multi_config_vec.push(Box::new(CommandLineVcl::new(vec!["".to_string(), key, value,]))),


Yeah, the need of putting the empty string at the beginning, always, is truly ugly. I believe it can be fixed without too big efforts.

bertllll · 2023-09-17T19:49:37Z

node/src/node_configurator/node_configurator_standard.rs

-        real_user,
-        data_directory,
-    ))
+    fill_specified_or_unspecified_box("--config-file".to_string(), config_file_path.to_string_lossy().to_string(), config_user_specified);


When you have the special structs holding the parameter and the user specified flag:

a) you can supply it here as a single thing, and get less params for this function.
b) you can go even further and give the struct another field holding the name of the parameter eg.: "--config-file".

If you go also with b) then you can make this function a method of that struct. It would look really compact.

bertllll · 2023-09-17T20:08:32Z

node/src/node_configurator/node_configurator_standard.rs

-    let config_file_vcl = match ConfigFileVcl::new(&config_file_path, user_specified) {
-        Ok(cfv) => Box::new(cfv),
-        Err(e) => return Err(ConfiguratorError::required("config-file", &e.to_string())),
+    let mut fill_specified_or_unspecified_box = |key: String, value: String, specified: bool | {


Again, nobody really cares what data structure serves as the medium (whether a Box or a Vec), he can read it from the code easily. To express the purpose is always helpful, though.

Maybe: sort_out_by_whether_user_specified.

bertllll · 2023-09-17T20:11:51Z

node/src/node_configurator/node_configurator_standard.rs

-    ))
+    fill_specified_or_unspecified_box("--config-file".to_string(), config_file_path.to_string_lossy().to_string(), config_user_specified);
+    fill_specified_or_unspecified_box("--data-directory".to_string(), data_directory.to_string_lossy().to_string(), data_directory_specified);
+    fill_specified_or_unspecified_box("--real-user".to_string(), real_user.to_string(), real_user_specified);


Again, a function call usually look better if without other function calls in the place of its arguments. You can enhance it here if you supply &str instead of the first argument. You will simply call the to_string() method inside the function.

bertllll · 2023-09-17T20:17:09Z

node/src/node_configurator/node_configurator_standard.rs

+    }
+
+    #[test]
+    fn server_initializer_collected_params_handle_config_file_from_environment_and_real_user_from_config_file_with_path_started_by_dot() {


"handleS"

You are mentioning the config file twice in the name, you can put the two parts together and get a simpler name.

You will want the Environment guard here too because this tests refers to the environment setting.

bertllll · 2023-09-17T20:20:40Z

node/src/node_configurator/node_configurator_standard.rs

+            .data_dir_result(Some(data_dir.to_path_buf()));
+
+        let result = server_initializer_collected_params(&dir_wrapper, args_vec.as_slice());
+        let env_multiconfig = result.unwrap();


Why these two lines if it can be easily done in one?

bertllll · 2023-09-17T20:22:55Z

node/src/node_configurator/node_configurator_standard.rs

+                    "/home/wooga/data_dir/MASQ/polygon-mainnet".to_string()
+                )
+            }
+        }


Yeah these assertions are really bad. Get rid of the variability of the test's behavior. You need it to behave always the same and you need it to match a single, concrete result (again...EnvironmentGuard)

bertllll · 2023-09-17T20:27:12Z

node/src/node_configurator/node_configurator_standard.rs

+        let home_dir = ensure_node_home_directory_exists( "node_configurator_standard","server_initializer_collected_params_handle_config_file_from_environment_and_real_user_from_config_file_with_path_started_by_dot");
+        let data_dir = &home_dir.join("data_dir");
+        let config_file_relative = File::create(PathBuf::from("./generated").join("config.toml")).unwrap();
+        fill_up_config_file(config_file_relative);


The test would be easier to understand if the reader saw what (supposed as different) real user value defines there. Maybe you could make it an argument your fill_up_config_file function and supply it from here. Then the reader can check both values easily. These seem important for this test.

bertllll · 2023-09-17T20:28:35Z

node/src/node_configurator/node_configurator_standard.rs

+    }
+
+    #[test]
+    fn server_initializer_collected_params_handle_config_file_from_environment_and_real_user_from_config_file_with_data_directory() {


"handleS"

The name stated like this isn't really clear can you restate it a bit?

bertllll · 2023-09-17T20:30:35Z

node/src/node_configurator/node_configurator_standard.rs

+                    &current_directory.to_string_lossy().to_string()
+                )
+            },
+            false => ()


This conditionality is wrong. Mentioned at the other tests...

bertllll · 2023-09-17T20:34:03Z

node/src/node_configurator/node_configurator_standard.rs

+        let env_vec_array = vec![
+            ("MASQ_CONFIG_FILE", "./generated/config.toml"),
+            #[cfg(not(target_os = "windows"))]
+            ("MASQ_REAL_USER", "9999:9999:booga"),


What role does this parameter plays here? If none big one, you should remove it. If some real than think about what will happen for this test when running on Windows... It will be missing.

Bu the way, Windows doesn't need to deal with this business around the real-user, never. Maybe your tests, and now I really talk in a plural, should not run on Windows completely, or some of these??? Do think about that please.

…ers_from_config_file, some optimization

…orted server_initializer_collected_params and determine_user_specific_data fncs, new fns value_opt for VclArg and tests for them

…ed_params, added handling for tilde in data_directory and config_file paths

…er_collected_params, fixed formatting for get_real_user_from_vcl

…zer_collected_params_handle_config_file_from_commandline_and_real_user_from_config_file_with_data_dir_started_by_tilde

…tiConfig, dealing with comments

czarte added 8 commits August 23, 2023 20:33

begining with test server_initializer_collected_params_combine_vlcs_p…

cedb1b3

…roperly

removed GatheredParams from server_initializer_collected_params, adde…

32dd2cb

…d bools for data_dir, config_file and real_user to MultiConfig, fixing the test

add ComputedVcl struct and its implementation, add VclType enum, to b…

690b099

…e able to push into Vcl Vector, add processing in MultiConfig, add processing in server_initializer_collected_params

back to VirtualCommandLine type without enum, passing different types…

0d7427b

… to MultiConfig and selecting them to computed_value_names on behalf of their type

fixed data flow with Evironment and Command line arguments, need to f…

22e607b

…ix config file arguments data

changes in test to access config.file for various scenarios, add mach…

b9a6e85

…inery to determine if real-user is specified in config-file

try to get rid of redundant MultiConfig construction in determine_use…

08349e2

…r_specific_data

handle computed arguments in multiconfig, closure for server_initiali…

8aead2a

…zer_collected_params, closure for keeping in node_configurator, final removal of redundant construction of multiconfig

czarte assigned dnwiebe Sep 15, 2023

dnwiebe reviewed Sep 15, 2023

View reviewed changes

bertllll reviewed Sep 15, 2023

View reviewed changes

bertllll reviewed Sep 17, 2023

View reviewed changes

czarte added 16 commits September 21, 2023 14:41

computing env_args and cmd_args in create_preorientation_args fn!

6167856

fix passing test server_initializer_collected_params_can_read_paramet…

7286ee8

…ers_from_config_file, some optimization

fixed VclArg problem with withdrawing data from VirtualCommandline, s…

fce6fe1

…orted server_initializer_collected_params and determine_user_specific_data fncs, new fns value_opt for VclArg and tests for them

finalize preparing guts for multiconfig in server_initializer_collect…

4dd0932

…ed_params, added handling for tilde in data_directory and config_file paths

fix windows test of real_user soecified

413ffc5

renaming, adding TODOs, removing resolved TODOS

18d0cc1

fixing tests for windows os

efd3fbb

fixed on windows

cf6da59

formatting

34b8fea

fixed real user for windows

5895879

removed filling specified and unspecified boxes from server_initializ…

62e99e3

…er_collected_params, fixed formatting for get_real_user_from_vcl

last fixes of extract_values_and_fill_boxes, and test server_initiali…

8369ff9

…zer_collected_params_handle_config_file_from_commandline_and_real_user_from_config_file_with_data_dir_started_by_tilde

fixed windows test for tilde

5b02822

fix tilde test for windows

0603f70

renaming of tests and functions

2ec7e12

remove GatheredParams (the goal), optimizing is_user_specified in Mul…

fe491ab

…tiConfig, dealing with comments

czarte closed this Nov 10, 2023

GH 731 review #341

GH 731 review #341

Conversation

czarte commented Sep 15, 2023

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

bertllll left a comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

bertllll Sep 15, 2023 • edited Loading

Choose a reason for hiding this comment

bertllll Sep 15, 2023 • edited Loading

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

bertllll Sep 15, 2023 •

edited

Loading

bertllll Sep 15, 2023 •

edited

Loading