QNTM-5869: Search keywords split mechanism by scottmitchell · Pull Request #9271 · DynamoDS/Dynamo

scottmitchell · 2018-11-27T22:02:08Z

Purpose

This PR addresses jira task QNTM-5869: Search keywords split mechanism.

This PR does:

Creates a new menu item under Settings > Experimental that toggles Experimental Search
Adds the full, unsplit search query to the list of queries in the search algorithm (when Experimental Search is ON)

Declarations

Check these if you believe they are true

The code base is in a better state after this PR
Is documented according to the standards
The level of testing this PR includes is appropriate
User facing strings, if any, are extracted into *.resx files
All tests pass using the self-service CI.
Snapshot of UI changes, if any.
Changes to the API follow Semantic Versioning, and are documented in the API Changes document.

Reviewers

@mjkkirschner @ColinDayOrg @Racel

…nto QNTM-5869

…-5869

scottmitchell · 2018-11-27T22:03:26Z

src/DynamoCoreWpf/Properties/Resources.en-US.resx

  <data name="PublishPackageViewPackageVersion" xml:space="preserve">
    <value>Version (major minor build)</value>
  </data>
-  <data name="DynamoViewSettingsMenuIsolationMode" xml:space="preserve">


I'm not sure where these resource changes are coming from

mjkkirschner · 2018-11-27T22:08:00Z

src/AssemblySharedInfoGenerator/AssemblySharedInfo.cs

 // in AssemblyVersionInfo.cs so that it can be easily incremented by the
 // automated build process.
-[assembly: AssemblyVersion("2.1.0.4395")]
+[assembly: AssemblyVersion("2.1.0.6957")]


you should usually not commit this file.

Whoops, reverting this

mjkkirschner · 2018-11-27T22:08:32Z

src/DynamoCore/Search/SearchDictionary.cs

        protected readonly Dictionary<V, Dictionary<string, double>> entryDictionary =
            new Dictionary<V, Dictionary<string, double>>();

+        public bool experimentalSearch = false;


can this be made private?

I don't think I can make it private here because I need to be able to access it from the DynamoViewModel. Maybe I'm missing something?

one option because this is an internal API is mark this internal and use [internalsVisibleAttribute] to make this property accessible from DynamoCoreWPF assembly.

mjkkirschner · 2018-11-27T22:09:33Z

src/DynamoCoreWpf/ViewModels/Core/DynamoViewModel.cs

            }
        }

+        public bool ExperimentalSearch


should have a summary /// tag if this a public API - does it need to be public?

ColinDayOrg · 2018-11-27T23:08:07Z

src/AssemblySharedInfoGenerator/AssemblySharedInfo.cs

 // by using the '*' as shown below:
 // [assembly: AssemblyVersion("1.0.*")]
-[assembly: AssemblyFileVersion("2.1.0.4395")]
+[assembly: AssemblyFileVersion("2.1.0.6957")]


I don't think that this file is usually included in PRs.

mjkkirschner · 2018-11-28T16:32:55Z

src/DynamoCoreWpf/ViewModels/Core/DynamoViewModel.cs

+            get { return model.SearchModel.experimentalSearch; }
+            set
+            {
+                model.SearchModel.experimentalSearch = value;


I guess in this case it's not such a big deal, but you usually would not modify a field from another object - I think instead we usually use a property, in this case I think it makes sense to try and hide this field from external consumers - we don't want anyone to use it - in an extension for example.

mjkkirschner · 2018-11-28T16:34:45Z

@scottmitchell some other thoughts -
have you looked at the debug menu?(debug menu only shows up in debug builds) This might actually be better than the experimental menu because it will not be discoverable to users... I don't know if that was @Racel 's intention or not.

You could also use an ifdef compile time flag using the debug config.

Racel · 2018-11-28T16:40:57Z

@mjkkirschner @scottmitchell Either menu works. Just need it in some menu for testing.... if it looks ok, we will probably just make it the standard.

mjkkirschner · 2018-11-28T16:51:19Z

If thats the case and its not really for users, but for us, my feeling is that it should go into the debug menu - I would also like to hide the API since it doesn't seem like something we want people using from extensions or packages.

Racel · 2018-11-28T16:56:50Z

@mjkkirschner Sounds good to me. Let's put it in the debug menu and make the API private

scottmitchell · 2018-11-28T20:09:46Z

I think this is ready to go

ColinDayOrg · 2018-11-28T20:46:58Z

LGTM
Thanks for changing from Add to Insert(0) as well. I know it doesn't matter right now, but it may in the future.

mjkkirschner · 2018-12-05T22:34:27Z

@scottmitchell is this waiting on something?

QilongTang · 2018-12-06T05:28:00Z

@mjkkirschner This is waiting on @Racel 's review in R2020

ColinDayOrg · 2018-12-10T19:16:01Z

src/DynamoCore/Search/SearchDictionary.cs

            query = query.ToLower();

            var subPatterns = SplitOnWhiteSpace(query);
+


Just as a question; should this code be wrapped in #if DEBUG statements? Probably not necessary, but I am doing the same type of code and wanted to get the group thought about if it is needed/wanted or not.

I think because this code is already in the debug only menu it's fine not to use a compiler flag. On the other hand, for performance impacting code - like Console.WriteLine I would use a debug flag:
https://stackoverflow.com/questions/18464833/console-writeline-effect-on-performance

ColinDayOrg · 2018-12-10T19:17:49Z

src/DynamoCore/Search/SearchDictionary.cs

+        {
+            get { return experimentalSearch; }
+            set { experimentalSearch = value;  }
+        }


When implementing my own debug mode VS suggested;
internal bool ExperimentalSearch { get; set; } = false;

@ColinDayOrg Yep, that's cleaner. Fixed.

mjkkirschner · 2018-12-14T02:26:57Z

@scottmitchell there's now conflicts on this branch from your other PR I think - might making @Racel 's review harder.

scottmitchell · 2018-12-14T15:41:17Z

@mjkkirschner Look's like it was caused by @ColinDayOrg 's PR. I'll fix.

scottmitchell · 2018-12-14T16:54:06Z

src/DynamoCoreWpf/Views/Core/DynamoView.xaml

                                  IsChecked="{Binding ShowDebugASTs}"></MenuItem>
                        <MenuItem Focusable="False"
-                                  Name="ExperimentalSearch"
+                                  Name="TruncateSearchResults"


@ColinDayOrg FYI I renamed your menu item name

Good catch, thanks. I am assuming that the "TruncateSearchResults" menu has been checked to test if it still works as expected after this change?

I just did a quick test to verify that the menu still works in the current master codebase after this specific change, so it seems ok to me.

ColinDayOrg · 2018-12-14T17:03:54Z

		query = query.ToLower();

		var subPatterns = SplitOnWhiteSpace(query);

Conversation

scottmitchell commented Nov 27, 2018 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Purpose

Declarations

Reviewers

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

mjkkirschner commented Nov 28, 2018 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Uh oh!

Racel commented Nov 28, 2018

Uh oh!

mjkkirschner commented Nov 28, 2018

Uh oh!

Racel commented Nov 28, 2018

Uh oh!

scottmitchell commented Nov 28, 2018

Uh oh!

ColinDayOrg commented Nov 28, 2018

Uh oh!

mjkkirschner commented Dec 5, 2018

Uh oh!

QilongTang commented Dec 6, 2018

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

mjkkirschner commented Dec 14, 2018

Uh oh!

scottmitchell commented Dec 14, 2018

Uh oh!

scottmitchell Dec 14, 2018 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

ColinDayOrg commented Dec 14, 2018

Uh oh!

Reviewers

Assignees

Labels

Projects

Milestone

Development

Uh oh!

6 participants

scottmitchell commented Nov 27, 2018 •

edited

Loading

mjkkirschner commented Nov 28, 2018 •

edited

Loading

scottmitchell Dec 14, 2018 •

edited

Loading