-
Notifications
You must be signed in to change notification settings - Fork 63
Add compare categories command #726
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
base: main
Are you sure you want to change the base?
Conversation
danielpgross
left a comment
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.
Approach looks good overall. What stood out:
- this approach can't recognize relocations, it will report them as archive + add new category. Maybe that's okay, but good to be aware of.
- Did you consider using mappings for this? They were built to capture basically this same information and are better because they capture relocations
We would also need unit tests added before merging
| end | ||
|
|
||
| desc "compare_categories VERSION_FOLDER", "Compare category changes between full_names.yml and categories.txt" | ||
| option :output_dir, type: :string, default: "exports", desc: "Output directory for CSV file" |
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.
It would be more flexible and idiomatic to accept the full output path of the CSV file (filename included), not just output dir
| AddValueCommand.new(options.merge(name:, attribute_friendly_id:)).run | ||
| end | ||
|
|
||
| desc "compare_categories VERSION_FOLDER", "Compare category changes between full_names.yml and categories.txt" |
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.
I think we can make the description clearer, maybe something like this:
| desc "compare_categories VERSION_FOLDER", "Compare category changes between full_names.yml and categories.txt" | |
| desc "compare_categories VERSION_FOLDER", "Generate a CSV report showing category changes between the current taxonomy and Shopify taxonomy VERSION" |
|
|
||
| desc "compare_categories VERSION_FOLDER", "Compare category changes between full_names.yml and categories.txt" | ||
| option :output_dir, type: :string, default: "exports", desc: "Output directory for CSV file" | ||
| def compare_categories(version_folder) |
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.
I find version_folder confusing here, isn't it just the version?
| def compare_categories(version_folder) | |
| def compare_categories(version) |
|
|
||
| # Ignore exports folder | ||
| /exports |
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.
We don't need to add an exports folder, the user can choose to export the CSV to wherever they want. Let's remove this.
Add category comparison command
This PR adds a new CLI command to compare category changes between different versions of the taxonomy.
What it does
full_names.ymlfrom a specific version folder againstdist/en/categories.txtexports/folderUsage
Files changed
dev/lib/product_taxonomy/commands/compare_categories_command.rbexports/folder to.gitignoreExample output
The command generates CSV files with columns:
type,id,old_name,new_name