READY : Allow to save nature order in commands & props #1325

SRetip · 2024-05-13T08:24:09Z

No description provided.

Wandalen · 2024-05-13T19:30:00Z

@Barsik-sus review please

Wandalen · 2024-05-21T05:12:27Z

@Barsik-sus review please

Barsik-sus

Does with_nature_sort work for both command and property order, or only for one of them?

Barsik-sus · 2024-05-22T09:20:21Z

module/move/wca/src/ca/aggregator.rs

@@ -117,7 +117,7 @@ pub( crate ) mod private

    #[ former( default = Verifier ) ]
    verifier : Verifier,
-
+    


Barsik-sus · 2024-05-22T09:28:48Z

module/move/wca/src/ca/grammar/command.rs

@@ -100,6 +99,8 @@ pub( crate ) mod private
    pub subjects : Vec< ValueDescription >,
    /// Hints and types for command options.
    pub properties : HashMap< String, ValueDescription >,
+    ///


Could you write concise documentation

Barsik-sus · 2024-05-22T09:31:05Z

module/move/wca/src/ca/help.rs

@@ -53,6 +53,11 @@ pub( crate ) mod private
    pub description_detailing : LevelOfDetail,
    /// If enabled - shows complete description of subjects and properties
    pub with_footer : bool,
+
+    order : Option< Vec< String > >,


Could you write concise documentation

Barsik-sus · 2024-05-22T09:34:24Z

module/move/wca/src/ca/help.rs

        LevelOfDetail::Detailed => command.properties.iter().map( |( n, v )| format!( "< {n}:{}{:?} >", if v.optional { "?" } else { "" }, v.kind ) ).collect::< Vec< _ > >().join( " " ),
      };

      let footer = if o.with_footer
      {
        let full_subjects = command.subjects.iter().map( | subj | format!( "- {} [{}{:?}]", subj.hint, if subj.optional { "?" } else { "" }, subj.kind ) ).join( "\n\t" );
-        let full_properties = format_table( command.properties.iter().sorted_by_key( |( name, _ )| *name ).map( |( name, value )| [ name.clone(), format!( "- {} [{}{:?}]", value.hint, if value.optional { "?" } else { "" }, value.kind ) ] ) ).unwrap().replace( '\n', "\n\t" );
+        let full_properties =  if o.with_nature_order


Suggested change

let full_properties = if o.with_nature_order

let full_properties = if o.with_nature_order

Barsik-sus · 2024-05-22T09:34:36Z

module/move/wca/src/ca/help.rs

-        let full_properties = format_table( command.properties.iter().sorted_by_key( |( name, _ )| *name ).map( |( name, value )| [ name.clone(), format!( "- {} [{}{:?}]", value.hint, if value.optional { "?" } else { "" }, value.kind ) ] ) ).unwrap().replace( '\n', "\n\t" );
+        let full_properties =  if o.with_nature_order
+        {
+          format_table( command.properties_order.iter().map( | name | [ name.clone(), format!( "- {} [{}{:?}]", command.properties.get( name ).unwrap().hint, if command.properties.get( name ).unwrap().optional { "?" } else { "" }, command.properties.get( name ).unwrap().kind ) ] ) ).unwrap().replace( '\n', "\n\t" )


I think we can avoid duplication here, can't we?

Barsik-sus · 2024-05-22T09:35:50Z

module/move/wca/src/ca/help.rs

-        let full_properties = format_table( command.properties.iter().sorted_by_key( |( name, _ )| *name ).map( |( name, value )| [ name.clone(), format!( "- {} [{}{:?}]", value.hint, if value.optional { "?" } else { "" }, value.kind ) ] ) ).unwrap().replace( '\n', "\n\t" );
+        let full_properties =  if o.with_nature_order
+        {
+          format_table( command.properties_order.iter().map( | name | [ name.clone(), format!( "- {} [{}{:?}]", command.properties.get( name ).unwrap().hint, if command.properties.get( name ).unwrap().optional { "?" } else { "" }, command.properties.get( name ).unwrap().kind ) ] ) ).unwrap().replace( '\n', "\n\t" )


Could you fmt this?

Barsik-sus · 2024-05-22T09:37:20Z

module/move/wca/src/ca/help.rs

    {
+      // debug_assert!( dictionary.commands.len() == order.as_ref().map( | o | o.len() ).unwrap_or( dictionary.commands.len() ) );


Remove redundant comments, please

Barsik-sus · 2024-05-22T09:45:32Z

module/move/wca/tests/inc/commands_aggregator/help.rs

@@ -59,3 +59,76 @@ wca = {{path = "{}"}}"#,
    result
  );
 }
+
+#[ test ]


I think It would be better if we refactor these tests to be able to see what the input is

That's not what I meant. Now we have to support this comment as well, and that's bad. I mean, we can modify the test to create the crate at the arrangement stage of a test

Barsik-sus · 2024-05-27T08:46:17Z

module/move/wca/src/ca/aggregator.rs

@@ -100,7 +100,7 @@ pub( crate ) mod private
  /// ```
  #[ derive( Debug ) ]
  #[ derive( former::Former ) ]
-  #[ storage_fields( help_generator : HelpGeneratorFn, help_variants : HashSet< HelpVariants > ) ]
+  #[ storage_fields( help_generator : HelpGeneratorFn, help_variants : HashSet< HelpVariants >, with_nature_sort : bool, order : Option< Vec< String > > ) ]


We can use enum instead of bool, can't we?

Barsik-sus

In my honest opinion, storing information about ordering in a dictionary is not the best idea, but if it works, I don't know how it would be better

Barsik-sus · 2024-05-28T09:01:16Z

module/move/wca/src/ca/formatter.rs

@@ -3,6 +3,7 @@ pub( crate ) mod private

  use crate::*;
  use wtools::Itertools;
+  use crate::ca::aggregator::private::Order;


This seems to be not in line with the convention

Barsik-sus · 2024-05-28T09:02:26Z

module/move/wca/src/ca/grammar/command.rs

  use former::{ Former, StoragePreform };
+  use crate::ca::aggregator::private::Order;


This seems to be not in line with the convention

Barsik-sus · 2024-05-28T09:03:19Z

module/move/wca/src/ca/grammar/dictionary.rs

  use former::Former;
+  use crate::ca::aggregator::private::Order;


This seems to be not in line with the convention

Barsik-sus · 2024-05-28T09:03:56Z

module/move/wca/src/ca/help.rs

@@ -12,6 +12,7 @@ pub( crate ) mod private
  use error_tools::for_app::anyhow;
  use former::Former;
  use ca::tool::table::format_table;
+  use crate::ca::aggregator::private::Order;


This seems to be not in line with the convention

Barsik-sus · 2024-05-28T09:04:18Z

module/move/wca/src/ca/verifier/verifier.rs

  use wtools::{ error, error::Result, err };
  use ca::help::private::{ HelpGeneratorOptions, LevelOfDetail, generate_help_content };
+  use crate::ca::grammar::dictionary::private::CommandName;


This seems to be not in line with the convention

Barsik-sus · 2024-05-28T09:13:41Z

module/move/wca/src/ca/grammar/command.rs

        let value = ValueDescription
        {
          hint : property.hint,
          kind : property.kind,
          optional : property.optional,
        };
        debug_assert!( !properties.contains_key( &property.name ), "Property name `{}` is already used for `{:?}`", property.name, properties[ &property.name ] );
-        properties.insert( property.name.clone(), value );
+        super_former.storage.last_id = Some( super_former.storage.last_id.unwrap_or_default() + 1 );
+        let name = CommandName{ id : super_former.storage.last_id.unwrap(), name : property.name.clone() };


Suggested change

let name = CommandName{ id : super_former.storage.last_id.unwrap(), name : property.name.clone() };

let name = CommandName { id : super_former.storage.last_id.unwrap(), name : property.name.clone() };

Barsik-sus · 2024-05-28T09:14:24Z

module/move/wca/src/ca/grammar/dictionary.rs

@@ -31,7 +85,9 @@ pub( crate ) mod private
    pub fn command( mut self, command : Command ) -> Self
    {
      let mut commands = self.storage.commands.unwrap_or_default();
-      commands.extend([( command.phrase.clone(), command )]);
+      self.storage.dictionary_last_id = Some( self.storage.dictionary_last_id.unwrap_or_default() + 1 );
+      let name = CommandName{ id : self.storage.dictionary_last_id.unwrap(), name : command.phrase.clone() };


Suggested change

let name = CommandName{ id : self.storage.dictionary_last_id.unwrap(), name : command.phrase.clone() };

let name = CommandName { id : self.storage.dictionary_last_id.unwrap(), name : command.phrase.clone() };

Barsik-sus · 2024-05-28T09:14:38Z

module/move/wca/src/ca/grammar/dictionary.rs

@@ -47,7 +103,9 @@ pub( crate ) mod private
    /// * `command` - The command to be registered.
    pub fn register( &mut self, command : Command ) -> Option< Command >
    {
-      self.commands.insert( command.phrase.clone(), command )
+      self.dictionary_last_id += 1;
+      let name = CommandName{ id : self.dictionary_last_id, name : command.phrase.clone() };


Suggested change

let name = CommandName{ id : self.dictionary_last_id, name : command.phrase.clone() };

let name = CommandName { id : self.dictionary_last_id, name : command.phrase.clone() };

Barsik-sus · 2024-05-28T09:15:21Z

module/move/wca/src/ca/grammar/dictionary.rs

    {
-      self.commands.get( name )
+      self.commands.iter().find( |(k, _)| k.name == name.to_string() ).map(|(_,  v)| v)


Suggested change

self.commands.iter().find( |(k, _)| k.name == name.to_string() ).map(|(_, v)| v)

self.commands.iter().find( |( k, _ )| k.name == name.to_string() ).map( |( _, v )| v )

Barsik-sus · 2024-05-28T09:16:20Z

module/move/wca/src/ca/help.rs

      };

      let footer = if o.with_footer
      {
        let full_subjects = command.subjects.iter().map( | subj | format!( "- {} [{}{:?}]", subj.hint, if subj.optional { "?" } else { "" }, subj.kind ) ).join( "\n\t" );
-        let full_properties = format_table( command.properties.iter().sorted_by_key( |( name, _ )| *name ).map( |( name, value )| [ name.clone(), format!( "- {} [{}{:?}]", value.hint, if value.optional { "?" } else { "" }, value.kind ) ] ) ).unwrap().replace( '\n', "\n\t" );
+        let full_properties = format_table( command.properties( dictionary.order ).into_iter().map( | ( name, value ) | [ name.clone(), format!( "- {} [{}{:?}]", value.hint, if value.optional { "?" } else { "" }, value.kind ) ] )).unwrap().replace( '\n', "\n\t" );


Suggested change

let full_properties = format_table( command.properties( dictionary.order ).into_iter().map( | ( name, value ) | [ name.clone(), format!( "- {} [{}{:?}]", value.hint, if value.optional { "?" } else { "" }, value.kind ) ] )).unwrap().replace( '\n', "\n\t" );

let full_properties = format_table( command.properties( dictionary.order ).into_iter().map( | ( name, value ) | [ name.clone(), format!( "- {} [{}{:?}]", value.hint, if value.optional { "?" } else { "" }, value.kind ) ] ) ).unwrap().replace( '\n', "\n\t" );

SRetip added 3 commits May 13, 2024 11:22

wip

6ef696f

Merge branch 'alpha' of https://github.com/Wandalen/wTools into alpha

e85ff9a

ready

e59ee4c

SRetip changed the title ~~NOT READY: Allow to save nature order~~ NOT READY: Allow to save nature order in commands & props May 13, 2024

SRetip added 2 commits May 13, 2024 13:54

remove dbg

c1b3a6e

fmt

039d5ab

SRetip changed the title ~~NOT READY: Allow to save nature order in commands & props~~ READY: Allow to save nature order in commands & props May 13, 2024

SRetip marked this pull request as ready for review May 13, 2024 10:56

Wandalen changed the title ~~READY: Allow to save nature order in commands & props~~ NOT REVIEWED: Allow to save nature order in commands & props May 13, 2024

Merge branch 'alpha' of https://github.com/Wandalen/wTools into alpha

e4cad1d

Barsik-sus suggested changes May 22, 2024

View reviewed changes

Wandalen changed the title ~~NOT REVIEWED: Allow to save nature order in commands & props~~ NOT READY : Allow to save nature order in commands & props May 27, 2024

SRetip added 2 commits May 27, 2024 11:02

Merge branch 'alpha' of https://github.com/Wandalen/wTools into alpha

7e9244c

after review fix

d551a3c

SRetip changed the title ~~NOT READY : Allow to save nature order in commands & props~~ READY : Allow to save nature order in commands & props May 27, 2024

SRetip requested a review from Barsik-sus May 27, 2024 08:35

Barsik-sus reviewed May 27, 2024

View reviewed changes

refactor

7279338

SRetip requested a review from Barsik-sus May 27, 2024 17:17

SRetip changed the title ~~READY : Allow to save nature order in commands & props~~ WAITING FOR REVIEW : Allow to save nature order in commands & props May 27, 2024

Barsik-sus suggested changes May 28, 2024

View reviewed changes

SRetip changed the title ~~WAITING FOR REVIEW : Allow to save nature order in commands & props~~ NOT READY : Allow to save nature order in commands & props May 28, 2024

SRetip added 2 commits May 28, 2024 18:16

import fix

e6ab345

test fix

22f9eac

SRetip changed the title ~~NOT READY : Allow to save nature order in commands & props~~ READY : Allow to save nature order in commands & props May 28, 2024

Wandalen merged commit 4f18593 into Wandalen:alpha May 29, 2024
1 of 4 checks passed

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

READY : Allow to save nature order in commands & props #1325

READY : Allow to save nature order in commands & props #1325

SRetip commented May 13, 2024

Wandalen commented May 13, 2024

Wandalen commented May 21, 2024

Barsik-sus left a comment

Barsik-sus May 22, 2024

Barsik-sus May 22, 2024

Barsik-sus May 22, 2024

Barsik-sus May 22, 2024

Barsik-sus May 22, 2024

Barsik-sus May 22, 2024

Barsik-sus May 22, 2024

Barsik-sus May 22, 2024

Barsik-sus May 28, 2024

Barsik-sus May 27, 2024

Barsik-sus left a comment

Barsik-sus May 28, 2024

Barsik-sus May 28, 2024

Barsik-sus May 28, 2024

Barsik-sus May 28, 2024

Barsik-sus May 28, 2024

Barsik-sus May 28, 2024

Barsik-sus May 28, 2024

Barsik-sus May 28, 2024

Barsik-sus May 28, 2024

Barsik-sus May 28, 2024

		@@ -117,7 +117,7 @@ pub( crate ) mod private

		#[ former( default = Verifier ) ]
		verifier : Verifier,

	let full_properties = if o.with_nature_order
	let full_properties = if o.with_nature_order

		{
		// debug_assert!( dictionary.commands.len() == order.as_ref().map( \| o \| o.len() ).unwrap_or( dictionary.commands.len() ) );

		use former::{ Former, StoragePreform };
		use crate::ca::aggregator::private::Order;

		use former::Former;
		use crate::ca::aggregator::private::Order;

	let name = CommandName{ id : super_former.storage.last_id.unwrap(), name : property.name.clone() };
	let name = CommandName { id : super_former.storage.last_id.unwrap(), name : property.name.clone() };

	let name = CommandName{ id : self.storage.dictionary_last_id.unwrap(), name : command.phrase.clone() };
	let name = CommandName { id : self.storage.dictionary_last_id.unwrap(), name : command.phrase.clone() };

	let name = CommandName{ id : self.dictionary_last_id, name : command.phrase.clone() };
	let name = CommandName { id : self.dictionary_last_id, name : command.phrase.clone() };

	self.commands.iter().find( \|(k, _)\| k.name == name.to_string() ).map(\|(_, v)\| v)
	self.commands.iter().find( \|( k, _ )\| k.name == name.to_string() ).map( \|( _, v )\| v )

	let full_properties = format_table( command.properties( dictionary.order ).into_iter().map( \| ( name, value ) \| [ name.clone(), format!( "- {} [{}{:?}]", value.hint, if value.optional { "?" } else { "" }, value.kind ) ] )).unwrap().replace( '\n', "\n\t" );
	let full_properties = format_table( command.properties( dictionary.order ).into_iter().map( \| ( name, value ) \| [ name.clone(), format!( "- {} [{}{:?}]", value.hint, if value.optional { "?" } else { "" }, value.kind ) ] ) ).unwrap().replace( '\n', "\n\t" );

READY : Allow to save nature order in commands & props #1325

READY : Allow to save nature order in commands & props #1325

Conversation

SRetip commented May 13, 2024

Wandalen commented May 13, 2024

Wandalen commented May 21, 2024

Barsik-sus 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

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

Barsik-sus 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

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