Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

QNTM-5870: When searching weights should be taken into account before string matching #9312

Merged
merged 17 commits into from
Dec 14, 2018

Conversation

ColinDayOrg
Copy link
Contributor

@ColinDayOrg ColinDayOrg commented Dec 12, 2018

Please Note:

  1. Before submitting the PR, please review How to Contribute to Dynamo
  2. Dynamo Team will meet 1x a month to review PRs found on Github (Issues will be handled separately)
  3. PRs will be reviewed from oldest to newest
  4. If a reviewed PR requires changes by the owner, the owner of the PR has 30 days to respond. If the PR has seen no activity by the next session, it will be either closed by the team or depending on its utility will be taken over by someone on the team
  5. PRs should use either Dynamo's default PR template or one of these other template options in order to be considered for review.
  6. PRs that do not have one of the Dynamo PR templates completely filled out with all declarations satisfied will not be reviewed by the Dynamo team.
  7. PRs made to the DynamoRevit repo will need to be cherry-picked into all the DynamoRevit Release branches that Dynamo supports. Contributors will be responsible for cherry-picking their reviewed commits to the other branches after a LGTM label is added to the PR.

Purpose

This PR returns the top 20 search results instead of all search results (which can be arbitrarily large, especially after only typing one letter into the search bar). It also adds some logging to output the time taken to search to the console when running a debug build. This logging was not put behind a debug mode as it is only one line of output in a large amount of logging that occurs when searching.

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.

image

Reviewers

@mjkkirschner

FYIs

@scottmitchell

@ColinDayOrg
Copy link
Contributor Author

ColinDayOrg commented Dec 12, 2018

Note: After running CI and EngOps tests without the debug mode (so there were only 20 results being returned) all tests passed successfully.

.Select(x => x.Key);
var orderedSearchDict = searchDict.OrderByDescending(x => x.Value);
#if DEBUG
//if (this.logger != null)
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

@ColinDayOrg did you mean to leave this commented out?

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Yes. Normally I would delete code like this, but it seemed that it could possibly be useful in the future. It can be removed if that would be preferable. It can output a lot of logging data and may not need to be used a lot which is why I commented it out.

Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

If these are debug mode only code, I prefer to leave them not commented and once we flip the debug mode, we can remove then.

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Due to this code possibly outputting a large amount of logging data if it were uncommented, and it not needing to be used currently, I have removed this code. It can always be put back if needed.

@@ -14,6 +15,10 @@ namespace Dynamo.Search
/// </summary>
public class NodeSearchModel : SearchLibrary<NodeSearchElement, NodeModel>
{
internal NodeSearchModel(ILogger logger = null) : base(logger)
{
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

even though these are internal - what do you think about adding some comments on why these constructors were added?

@ColinDayOrg ColinDayOrg changed the title [WIP][DNM] QNTM-5870: When searching weights should be taken into account before string matching QNTM-5870: When searching weights should be taken into account before string matching Dec 12, 2018
@mjkkirschner
Copy link
Member

@ColinDayOrg this looks good - thanks for comments.
LGTM

@mjkkirschner mjkkirschner added the LGTM Looks good to me label Dec 14, 2018
@ColinDayOrg ColinDayOrg merged commit 6925ebb into DynamoDS:master Dec 14, 2018
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
LGTM Looks good to me
Projects
None yet
Development

Successfully merging this pull request may close these issues.

3 participants