-
Notifications
You must be signed in to change notification settings - Fork 16
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
Tweak the PR comment for consistency when there are no new mappings generated #401
base: main
Are you sure you want to change the base?
Conversation
generate_missing_mappings | ||
if @unmapped_categories_groups.empty? | ||
write_summary_message( | ||
all_generated_mappings: [], disagree_messages: [], total_count: 0, current_iteration_count: 0 |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Suggest renaming disagree_messages
to something like low_confidence_mappings
all_generated_mappings: [], disagree_messages: [], total_count: 0, current_iteration_count: 0 | |
all_generated_mappings: [], low_confidence_mappings: [], total_count: 0, num_mappings_generated: 0 |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Is it possible for the system to have 0 recommendation for a missing mapping? i.e. can we have a scenario where all_generated_mappings.size
is less than current_iteration_count
? If yes, then we should include a third section where we tell the user that we were unable to generate mappings for the categories that are still missing (and list the ones that are missing).
Looking at how we use this, a better name for
current_iteration_count
would be something likestarting_num_of_unmapped_categories
current_iteration_count: unmapped_categories[:id_name_map].size,
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
can we have a scenario where all_generated_mappings.size is less than current_iteration_count
No, semantic search will consistently provide some results based on proximity. And this question is thought-provoking, we can get rid of current_iteration_count
entirely because all_generated_mappings.length always == current_iteration_count
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Renamed parameters and removed redundant one.
2f1b922
to
e8a7603
Compare
e8a7603
to
beefc1e
Compare
beefc1e
to
a6509c1
Compare
Minor adjustment to keep the comment style consistent if there are no new mappings generated.
Previously, if new mappings were generated, the comment appeared as:
With the update, it will now read: