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

Tweak the PR comment for consistency when there are no new mappings generated #401

Open
wants to merge 3 commits into
base: main
Choose a base branch
from

Conversation

chesterbot01
Copy link
Contributor

@chesterbot01 chesterbot01 commented Oct 9, 2024

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:

Found 0 unmapped Shopify category.

With the update, it will now read:

## 🤖 Automatic Taxonomy Mapping Update
No unmapped Shopify categories were found. As a result, **no changes have been made to this PR.**

generate_missing_mappings
if @unmapped_categories_groups.empty?
write_summary_message(
all_generated_mappings: [], disagree_messages: [], total_count: 0, current_iteration_count: 0
Copy link
Contributor

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

Suggested change
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

Copy link
Contributor

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 like starting_num_of_unmapped_categories

current_iteration_count: unmapped_categories[:id_name_map].size,

Copy link
Contributor Author

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

Copy link
Contributor Author

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.

@chesterbot01 chesterbot01 force-pushed the update-the-comment-when-no-mapping-generated branch 2 times, most recently from 2f1b922 to e8a7603 Compare October 9, 2024 20:03
@chesterbot01 chesterbot01 force-pushed the update-the-comment-when-no-mapping-generated branch from e8a7603 to beefc1e Compare November 11, 2024 16:51
@chesterbot01 chesterbot01 force-pushed the update-the-comment-when-no-mapping-generated branch from beefc1e to a6509c1 Compare November 11, 2024 17:37
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

2 participants