Trim white space, compare and update sale rates.#51
Trim white space, compare and update sale rates.#51chuckbucket wants to merge 4 commits intoZZJHONS:masterfrom
Conversation
Trimming white space when comparing BL and BO items. Will now consider "c-03" and "c-03 " the same. BrickOwl appears to always trim whitespace, this would cause BrickSync to always try to update BrickOwl's inventory.
compare and update sale rates.
![gemini-code-assist[bot]](https://avatars.githubusercontent.com/in/956858?s=60&v=4){kind=small}
There was a problem hiding this comment.
Summary of Changes
Hello @chuckbucket, I'm Gemini Code Assist1! I'm currently reviewing this pull request and will post my feedback shortly. In the meantime, here's a summary to help you and other reviewers quickly get up to speed!
This pull request focuses on improving the accuracy and completeness of data synchronization. It addresses an issue where whitespace differences in text fields led to redundant updates and extends the synchronization capabilities to include sales rates for Brick Owl items, enhancing overall data consistency and reducing unnecessary API calls.
Highlights
- Whitespace Trimming for Comparisons: I've introduced a new utility function,
ccStrTrimWhitespace, which removes leading and trailing whitespace from a given string. This function is now applied when comparingcommentsandremarksfields between inventory items. This change ensures that differences solely due to varying whitespace (e.g., 'C-02' vs. 'C-02 ') no longer trigger unnecessary updates, aligning Bricksync's behavior with platforms like Brick Owl. - Sales Rate Synchronization for Brick Owl: The pull request enables the synchronization of
sale_percent(sales rates) for items when interacting with Brick Owl. Previously, this attribute was excluded from synchronization. Now, Bricksync will compare the sales rates and initiate updates as needed, ensuring this critical pricing information remains consistent across platforms.
Using Gemini Code Assist
The full guide for Gemini Code Assist can be found on our documentation page, here are some quick tips.
Invoking Gemini
You can request assistance from Gemini at any point in your pull request via creating an issue comment (i.e. comment on the pull request page) using either /gemini <command> or @gemini-code-assist <command>. Below is a summary of the supported commands.
| Feature | Command | Description |
|---|---|---|
| Code Review | /gemini review |
Performs a code review for the current pull request in its current state. |
| Pull Request Summary | /gemini summary |
Provides a summary of the current pull request in its current state. |
| Comment | @gemini-code-assist | Responds in comments when explicitly tagged, both in issue comments and review comments. |
| Help | /gemini help |
Displays a list of available commands. |
Customization
To customize Gemini Code Assist for GitHub experience, repository maintainers can create a configuration file and/or provide a custom code review style guide (such as PEP-8 for Python) by creating and adding files to a .gemini/ folder in the base of the repository. Detailed instructions can be found here.
Limitations & Feedback
Gemini Code Assist is currently in preview and may make mistakes. Please leave feedback on any instances where its feedback is incorrect or counter productive. You can react with 👍 and 👎 on @gemini-code-assist comments to provide feedback.
You can also get AI-powered code generation, chat, as well as code reviews directly in the IDE at no cost with the Gemini Code Assist IDE Extension.
Footnotes
-
Review the Privacy Notices, Generative AI Prohibited Use Policy, Terms of Service, and learn how to configure Gemini Code Assist in GitHub here. Gemini can make mistakes, so double check it and use code with caution. ↩
There was a problem hiding this comment.
Code Review
This pull request introduces two main features: trimming whitespace from comments and remarks before comparison, and adding support for updating sale rates. The implementation for updating sale rates seems correct and is consistently applied. The new whitespace trimming function ccStrTrimWhitespace in ccstr.c, however, has a side-effect bug where it modifies the input string, which could lead to unexpected behavior. I've suggested a fix for this. Additionally, there are some unused variables in bssync.c that should be cleaned up. Overall, the changes are in line with the PR's objectives, but the identified issues should be addressed.
| char *ccStrTrimWhitespace( char *str ) | ||
| { | ||
| char *end; | ||
| char *new_str; | ||
| size_t len; | ||
|
|
||
| if( str == NULL ) | ||
| return NULL; | ||
|
|
||
| // Trim leading space | ||
| while( isspace( (unsigned char)*str ) ) | ||
| str++; | ||
|
|
||
| if( *str == 0 ) // All spaces? | ||
| return ccStrDup( str ); | ||
|
|
||
| // Trim trailing space | ||
| end = str + strlen(str) - 1; | ||
| while( end > str && isspace( (unsigned char)*end ) ) | ||
| end--; | ||
|
|
||
| // Write new null terminator character | ||
| *(end+1) = 0; | ||
|
|
||
| len = (end - str) + 1; | ||
| new_str = (char *)malloc( len + 1 ); | ||
| if( new_str == NULL ) | ||
| return NULL; // Allocation failed | ||
|
|
||
| memcpy( new_str, str, len ); | ||
| new_str[len] = 0; | ||
|
|
||
| return new_str; | ||
| } |
There was a problem hiding this comment.
The current implementation of ccStrTrimWhitespace has a side effect of modifying the input string str. The line *(end+1) = 0; writes a null terminator into the buffer that str points to. This is likely unintended, as it can lead to subtle bugs if the caller expects the original string to remain unchanged (for example, for logging purposes).
The function should not modify its input; it should only return a new, trimmed string. I've suggested a fix that avoids this side effect by using a separate pointer to track the start of the trimmed string.
char *ccStrTrimWhitespace( char *str )
{
char *start = str;
char *end;
char *new_str;
size_t len;
if( str == NULL )
return NULL;
// Trim leading space
while( isspace( (unsigned char)*start ) )
start++;
if( *start == 0 ) // All spaces?
return ccStrDup( start );
// Trim trailing space
end = start + strlen(start) - 1;
while( end > start && isspace( (unsigned char)*end ) )
end--;
// Allocate and copy the trimmed string
len = (end - start) + 1;
new_str = (char *)malloc( len + 1 );
if( new_str == NULL )
return NULL; // Allocation failed
memcpy( new_str, start, len );
new_str[len] = 0;
return new_str;
}
bssync.c
Outdated
| char *item_comments_trimmed = NULL; | ||
| char *stockitem_comments_trimmed = NULL; | ||
| char *item_remarks_trimmed = NULL; | ||
| char *stockitem_remarks_trimmed = NULL; |
There was a problem hiding this comment.
These variables (item_comments_trimmed, stockitem_comments_trimmed, item_remarks_trimmed, stockitem_remarks_trimmed) are declared but are not used within the bsMergeInv function. This appears to be leftover code and should be removed to improve clarity.
|
Wouldn't be better if the space would actually be trimmed from Bricklink instead? Also having sales not sync was by design as some stores prefer to run sales independently, but maybe we can add this with a setting in the config file? Thanks for your work! |
That is a good point, I will move the trimming to happen during the parsing stage. I am not very familiar with C, I hope that I am not messing things up or adding memory leaks.
Good point. I will incorporate this. I will set the default to not sync the sales rates. |
Trims white spaced when parsing download from bricklink. Removed trim functions from sync function since they are now redundent.
Added a new setting in the config file. syncsalesrates: 1:yes 0: no (Default)
|
1.When it received and parses the bricklink data it will trim the whitespace from the comments and remarks.
|
|
@ZZJHONS, I believe that I addressed your comments with the latest two commits to the. I have been running a version with my changes are it seams to be running without issue. |
2 participants
1: White space
Brick Owl appears to always trim white space from comments and remarks. Bricklink does not. This would cause Bricksync to always try and update a remark or comment when comparing "C-02" and "C-02 ". Bricksync will now see them as the same and not try to update the lot.
2: Compare and update sales rates.
Bricksync will now look at the sales rates and make updates as needed. This was previously excluded and would not be updated during syncing.