-
Notifications
You must be signed in to change notification settings - Fork 28
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
feat: Get active counts of attribute #92
Conversation
Caution Review failedThe pull request is closed. WalkthroughThe changes involve enhancements to the error handling in the Changes
Suggested reviewers
Poem
Thank you for using CodeRabbit. We offer it for free to the OSS community and would appreciate your support in helping us grow. If you find it useful, would you consider giving us a shout-out on your favorite social media? 🪧 TipsChatThere are 3 ways to chat with CodeRabbit:
Note: Be mindful of the bot's finite context window. It's strongly recommended to break down tasks such as reading entire modules into smaller chunks. For a focused discussion, use review comments to chat about specific files and their changes, instead of using the PR comments. CodeRabbit Commands (Invoked using PR comments)
Other keywords and placeholders
CodeRabbit Configuration 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.
Actionable comments posted: 1
🧹 Outside diff range and nitpick comments (1)
sageintacctsdk/apis/api_base.py (1)
Line range hint
1-605
: Summary of changes and recommendations
The error handling improvement in the
__post_request
method is a positive change that enhances the SDK's ability to provide detailed feedback.The modification to the
count
method to only count active records is a significant change that may affect existing implementations. To address this:
- Consider making the active status filter optional through a method parameter.
- Update the method name and/or docstring to clearly reflect its behavior.
- Ensure that this change is documented in the SDK's changelog and release notes.
General recommendation: When making changes that alter the behavior of public methods, always consider backwards compatibility and provide clear documentation of the changes.
📜 Review details
Configuration used: CodeRabbit UI
Review profile: CHILL
📒 Files selected for processing (1)
- sageintacctsdk/apis/api_base.py (2 hunks)
🔇 Additional comments (1)
sageintacctsdk/apis/api_base.py (1)
204-204
: LGTM: Improved error handlingThis addition enhances the error handling by raising a
WrongParamsError
with a specific exception message when the response status indicates a failure. It's consistent with the existing error handling pattern and provides more detailed feedback to the user.
'filter': { | ||
'equalto': {'field': 'STATUS', 'value': 'active'} | ||
}, |
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.
Consider the impact of filtering only active records
The addition of a filter to only count active records is a significant change in the behavior of the count
method. While this may be the desired functionality, it has several implications:
- It changes the expected behavior of the method without updating the method signature or docstring.
- Existing code that relies on this method to get a total count of all records (including inactive ones) may now produce incorrect results.
Consider the following improvements:
- Update the method name to reflect its new behavior (e.g.,
count_active
). - Add a parameter to make the active status filter optional, allowing users to choose whether they want to count all records or only active ones.
- Update the method's docstring to clearly explain the behavior and any parameters.
Example implementation:
def count(self, active_only: bool = True):
"""
Count the number of records.
:param active_only: If True, only count active records. If False, count all records. Default is True.
:return: The count of records.
"""
get_count = {
'query': {
'object': self.__dimension,
'select': {
'field': 'RECORDNO'
},
'pagesize': '1'
}
}
if active_only:
get_count['query']['filter'] = {
'equalto': {'field': 'STATUS', 'value': 'active'}
}
response = self.format_and_send_request(get_count)
return int(response['data']['@totalcount'])
This approach maintains backwards compatibility while allowing for the new functionality.
@Ashutosh619-sudo pls bump version as well |
https://app.clickup.com/1864988/v/l/6-901603904304-1
Summary by CodeRabbit
New Features
Bug Fixes
Chores