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

Added generator in sdk and modified the calls in api #165

Merged
merged 8 commits into from
May 6, 2024
Merged
Show file tree
Hide file tree
Changes from 3 commits
Commits
File filter

Filter by extension

Filter by extension

Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
8 changes: 5 additions & 3 deletions apps/sage300/models.py
Original file line number Diff line number Diff line change
@@ -1,4 +1,3 @@
from typing import Dict, List
from django.db import models

from fyle_accounting_mappings.models import (
Expand All @@ -10,6 +9,7 @@
StringNotNullField,
BooleanFalseField
)
from sage_desktop_sdk.core.schema.read_only import Category


class CostCategory(BaseForeignWorkspaceModel):
Expand All @@ -30,14 +30,16 @@ class Meta:
db_table = 'cost_category'

@staticmethod
def bulk_create_or_update(categories_generator: List[Dict], workspace_id: int):
def bulk_create_or_update(categories_generator, workspace_id: int):
"""
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

we should not pass generator, we should pass current page items here. Line 41 ka category value for example

Bulk create or update cost types
"""

list_of_categories = []
for categories in categories_generator:
list_of_categories.append(categories)
for category in categories:
for data in category:
list_of_categories.append(Category.from_dict(data))

record_number_list = [category.id for category in list_of_categories]

Expand Down
61 changes: 48 additions & 13 deletions apps/sage300/utils.py
Original file line number Diff line number Diff line change
@@ -1,3 +1,4 @@
from django.utils.module_loading import import_string
from fyle_accounting_mappings.models import DestinationAttribute
from apps.workspaces.models import Sage300Credential
from sage_desktop_sdk.sage_desktop_sdk import SageDesktopSDK
Expand Down Expand Up @@ -66,7 +67,42 @@ def _update_latest_version(self, attribute_type: str):

return []

def _sync_data(self, data, attribute_type, display_name, workspace_id, field_names):
def _add_to_destination_attributes(self, item, attribute_type, display_name, field_names):
destination_attributes = []

detail = {field: getattr(item, field) for field in field_names}
if item.name:
destination_attributes.append(self._create_destination_attribute(
attribute_type,
display_name,
" ".join(item.name.split()),
item.id,
item.is_active,
detail
))

return destination_attributes

Copy link

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

The method _add_to_destination_attributes efficiently constructs destination attributes, but consider handling potential attribute errors when accessing properties of item.

        detail = {field: getattr(item, field) for field in field_names}
+       # Handle potential AttributeError if item does not have the field
+       detail = {field: getattr(item, field, None) for field in field_names}

Committable suggestion

‼️ IMPORTANT
Carefully review the code before committing. Ensure that it accurately replaces the highlighted code, contains no missing lines, and has no issues with indentation.

Suggested change
def _add_to_destination_attributes(self, item, attribute_type, display_name, field_names):
destination_attributes = []
detail = {field: getattr(item, field) for field in field_names}
if item.name:
destination_attributes.append(self._create_destination_attribute(
attribute_type,
display_name,
" ".join(item.name.split()),
item.id,
item.is_active,
detail
))
return destination_attributes
def _add_to_destination_attributes(self, item, attribute_type, display_name, field_names):
destination_attributes = []
# Handle potential AttributeError if item does not have the field
detail = {field: getattr(item, field, None) for field in field_names}
if item.name:
destination_attributes.append(self._create_destination_attribute(
attribute_type,
display_name,
" ".join(item.name.split()),
item.id,
item.is_active,
detail
))
return destination_attributes

def _get_attribute_class(self, attribute_type: str):
"""
Get the attribute class for the attribute type
:param attribute_type: Type of the attribute
:return: Attribute class
"""
ATTRIBUTE_CLASS_MAP = {
'ACCOUNT': 'Account',
'VENDOR': 'Vendor',
'JOB': 'Job',
'STANDARD_COST_CODE': 'StandardCostCode',
'STANDARD_CATEGORY': 'StandardCategory',
'COMMITMENT': 'Commitment',
'COMMITMENT_ITEM': 'CommitmentItem',
'COST_CODE': 'CostCode'
}

return ATTRIBUTE_CLASS_MAP[attribute_type]
Comment on lines +83 to +100
Copy link

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

The method _get_attribute_class uses a hard-coded mapping which could be externalized to a configuration file or database for easier maintenance.

Consider externalizing the ATTRIBUTE_CLASS_MAP to a configuration file or database to facilitate easier updates and maintenance without needing to modify the code.


def _sync_data(self, data_gen, attribute_type, display_name, workspace_id, field_names, is_gen: bool = True):
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

is_generator

"""
Synchronize data from Sage Desktop SDK to your application
:param data: Data to synchronize
Expand All @@ -77,17 +113,16 @@ def _sync_data(self, data, attribute_type, display_name, workspace_id, field_nam
"""

destination_attributes = []
for item in data:
detail = {field: getattr(item, field) for field in field_names}
if item.name:
destination_attributes.append(self._create_destination_attribute(
attribute_type,
display_name,
" ".join(item.name.split()),
item.id,
item.is_active,
detail
))
if is_gen:
for data in data_gen:
for items in data:
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

same comment as above

for _item in items:
attribute_class = self._get_attribute_class(attribute_type)
item = import_string(f'sage_desktop_sdk.core.schema.read_only.{attribute_class}').from_dict(_item)
destination_attributes = self._add_to_destination_attributes(item, attribute_type, display_name, field_names)
else:
for item in data_gen:
destination_attributes = self._add_to_destination_attributes(item, attribute_type, display_name, field_names)

DestinationAttribute.bulk_create_or_update_destination_attributes(
destination_attributes, attribute_type, workspace_id, True)
Expand Down Expand Up @@ -177,7 +212,7 @@ def sync_commitment_items(self):
'code', 'version', 'description', 'cost_code_id',
'category_id', 'created_on_utc', 'job_id', 'commitment_id'
]
self._sync_data(commitment_items, 'COMMITMENT_ITEM', 'commitment_item', self.workspace_id, field_names)
self._sync_data(commitment_items, 'COMMITMENT_ITEM', 'commitment_item', self.workspace_id, field_names, False)

def sync_cost_codes(self):
"""
Expand Down
13 changes: 5 additions & 8 deletions sage_desktop_sdk/apis/accounts.py
Original file line number Diff line number Diff line change
Expand Up @@ -2,7 +2,6 @@
Sage Desktop Categories
"""
from sage_desktop_sdk.core.client import Client
from sage_desktop_sdk.core.schema.read_only import Account


class Accounts(Client):
Expand All @@ -18,14 +17,12 @@ def get_all(self, version: int = None):
:return: A generator yielding accounts in the Attachable Schema
:rtype: generator of Account objects
"""
endpoint = Accounts.GET_ACCOUNTS + '?page={0}'
if version:
# Append the version query parameter if provided
query_params = '?version={0}'.format(version)
endpoint = Accounts.GET_ACCOUNTS + query_params
else:
endpoint = Accounts.GET_ACCOUNTS
query_params = f'&version={version}'
endpoint += query_params

# Query the API to get all accounts
accounts = self._query_get_all(endpoint)
for account in accounts:
yield Account.from_dict(account)
accounts = self._query_get_all_generator(endpoint, is_paginated=True)
yield accounts
Comment on lines +20 to +28
Copy link

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Ensure consistent use of string formatting for query parameters.

- endpoint = Accounts.GET_ACCOUNTS + '?page={0}'
+ endpoint = Accounts.GET_ACCOUNTS + f'?page={0}'

This change ensures that the string formatting is consistent with the rest of the code, which uses f-string formatting.


Committable suggestion

‼️ IMPORTANT
Carefully review the code before committing. Ensure that it accurately replaces the highlighted code, contains no missing lines, and has no issues with indentation.

Suggested change
endpoint = Accounts.GET_ACCOUNTS + '?page={0}'
if version:
# Append the version query parameter if provided
query_params = '?version={0}'.format(version)
endpoint = Accounts.GET_ACCOUNTS + query_params
else:
endpoint = Accounts.GET_ACCOUNTS
query_params = f'&version={version}'
endpoint += query_params
# Query the API to get all accounts
accounts = self._query_get_all(endpoint)
for account in accounts:
yield Account.from_dict(account)
accounts = self._query_get_all_generator(endpoint, is_paginated=True)
yield accounts
endpoint = Accounts.GET_ACCOUNTS + f'?page={0}'
if version:
# Append the version query parameter if provided
query_params = f'&version={version}'
endpoint += query_params
# Query the API to get all accounts
accounts = self._query_get_all_generator(endpoint, is_paginated=True)
yield accounts

Comment on lines +27 to +28
Copy link

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Yielding a generator object directly may not be the intended behavior.

- yield accounts
+ for account in accounts:
+     yield account

Directly yielding the generator object from _query_get_all_generator might not work as intended. Instead, iterate over the generator and yield each account individually.


Committable suggestion

‼️ IMPORTANT
Carefully review the code before committing. Ensure that it accurately replaces the highlighted code, contains no missing lines, and has no issues with indentation.

Suggested change
accounts = self._query_get_all_generator(endpoint, is_paginated=True)
yield accounts
accounts = self._query_get_all_generator(endpoint, is_paginated=True)
for account in accounts:
yield account

15 changes: 5 additions & 10 deletions sage_desktop_sdk/apis/categories.py
Original file line number Diff line number Diff line change
Expand Up @@ -2,7 +2,6 @@
Sage Desktop Categories
"""
from sage_desktop_sdk.core.client import Client
from sage_desktop_sdk.core.schema.read_only import Category


class Categories(Client):
Expand All @@ -20,16 +19,12 @@ def get_all_categories(self, version: int = None):
:return: A generator yielding job categories in the Jobs Schema
:rtype: generator of Category objects
"""
endpoint = Categories.GET_CATEGORIES + '?page={0}'
if version:
# Append the version query parameter if provided
query_params = '?version={0}'.format(version)
endpoint = Categories.GET_CATEGORIES + query_params
else:
endpoint = Categories.GET_CATEGORIES
query_params = f'&version={version}'
endpoint += query_params

# Query the API to get all job categories
categories = self._query_get_all(endpoint)

for category in categories:
# Convert each category dictionary to a Category object and yield it
yield Category.from_dict(category)
categories = self._query_get_all_generator(endpoint, is_paginated=True)
yield categories
Comment on lines +22 to +30
Copy link

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Ensure consistent use of string formatting for query parameters.

- endpoint = Categories.GET_CATEGORIES + '?page={0}'
+ endpoint = Categories.GET_CATEGORIES + f'?page={0}'

This change ensures that the string formatting is consistent with the rest of the code, which uses f-string formatting.


Committable suggestion

‼️ IMPORTANT
Carefully review the code before committing. Ensure that it accurately replaces the highlighted code, contains no missing lines, and has no issues with indentation.

Suggested change
endpoint = Categories.GET_CATEGORIES + '?page={0}'
if version:
# Append the version query parameter if provided
query_params = '?version={0}'.format(version)
endpoint = Categories.GET_CATEGORIES + query_params
else:
endpoint = Categories.GET_CATEGORIES
query_params = f'&version={version}'
endpoint += query_params
# Query the API to get all job categories
categories = self._query_get_all(endpoint)
for category in categories:
# Convert each category dictionary to a Category object and yield it
yield Category.from_dict(category)
categories = self._query_get_all_generator(endpoint, is_paginated=True)
yield categories
endpoint = Categories.GET_CATEGORIES + f'?page={0}'
if version:
# Append the version query parameter if provided
query_params = f'&version={version}'
endpoint += query_params
# Query the API to get all job categories
categories = self._query_get_all_generator(endpoint, is_paginated=True)
yield categories

Comment on lines +29 to +30
Copy link

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Yielding a generator object directly may not be the intended behavior.

- yield categories
+ for category in categories:
+     yield category

Directly yielding the generator object from _query_get_all_generator might not work as intended. Instead, iterate over the generator and yield each category individually.


Committable suggestion

‼️ IMPORTANT
Carefully review the code before committing. Ensure that it accurately replaces the highlighted code, contains no missing lines, and has no issues with indentation.

Suggested change
categories = self._query_get_all_generator(endpoint, is_paginated=True)
yield categories
categories = self._query_get_all_generator(endpoint, is_paginated=True)
for category in categories:
yield category

18 changes: 7 additions & 11 deletions sage_desktop_sdk/apis/commitments.py
Original file line number Diff line number Diff line change
Expand Up @@ -3,7 +3,7 @@
Sage Desktop Commitments
"""
from sage_desktop_sdk.core.client import Client
from sage_desktop_sdk.core.schema.read_only import Commitment, CommitmentItem
from sage_desktop_sdk.core.schema.read_only import CommitmentItem


class Commitments(Client):
Expand All @@ -22,19 +22,15 @@ def get_all(self, version: int = None):
:return: A generator yielding commitments in the Commitments Schema
:rtype: generator of Commitment objects
"""
endpoint = Commitments.GET_COMMITMENTS + '?page={0}'
if version:
# Append the version query parameter if provided
query_params = '?version={0}'.format(version)
endpoint = Commitments.GET_COMMITMENTS + query_params
else:
endpoint = Commitments.GET_COMMITMENTS
query_params = f'&version={version}'
endpoint += query_params

# Query the API to get all commitments
commitments = self._query_get_all(endpoint)

for commitment in commitments:
# Convert each commitment dictionary to a Commitment object and yield it
yield Commitment.from_dict(commitment)
commitments = self._query_get_all_generator(endpoint)
yield commitments
Comment on lines +25 to +33
Copy link

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Ensure consistent use of string formatting for query parameters.

- endpoint = Commitments.GET_COMMITMENTS + '?page={0}'
+ endpoint = Commitments.GET_COMMITMENTS + f'?page={0}'

This change ensures that the string formatting is consistent with the rest of the code, which uses f-string formatting.


Committable suggestion

‼️ IMPORTANT
Carefully review the code before committing. Ensure that it accurately replaces the highlighted code, contains no missing lines, and has no issues with indentation.

Suggested change
endpoint = Commitments.GET_COMMITMENTS + '?page={0}'
if version:
# Append the version query parameter if provided
query_params = '?version={0}'.format(version)
endpoint = Commitments.GET_COMMITMENTS + query_params
else:
endpoint = Commitments.GET_COMMITMENTS
query_params = f'&version={version}'
endpoint += query_params
# Query the API to get all commitments
commitments = self._query_get_all(endpoint)
for commitment in commitments:
# Convert each commitment dictionary to a Commitment object and yield it
yield Commitment.from_dict(commitment)
commitments = self._query_get_all_generator(endpoint)
yield commitments
endpoint = Commitments.GET_COMMITMENTS + f'?page={0}'
if version:
# Append the version query parameter if provided
query_params = f'&version={version}'
endpoint += query_params
# Query the API to get all commitments
commitments = self._query_get_all_generator(endpoint)
yield commitments

Comment on lines +32 to +33
Copy link

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Yielding a generator object directly may not be the intended behavior.

- yield commitments
+ for commitment in commitments:
+     yield commitment

Directly yielding the generator object from _query_get_all_generator might not work as intended. Instead, iterate over the generator and yield each commitment individually.


Committable suggestion

‼️ IMPORTANT
Carefully review the code before committing. Ensure that it accurately replaces the highlighted code, contains no missing lines, and has no issues with indentation.

Suggested change
commitments = self._query_get_all_generator(endpoint)
yield commitments
commitments = self._query_get_all_generator(endpoint)
for commitment in commitments:
yield commitment


def get_commitment_items(self, commitment_id: str, version: int = None):
"""
Expand All @@ -58,4 +54,4 @@ def get_commitment_items(self, commitment_id: str, version: int = None):
commitment_items = self._query_get_by_id(endpoint)

for commitment_item in commitment_items:
yield CommitmentItem.from_dict(commitment_item)
yield CommitmentItem.from_dict(commitment_item)
15 changes: 5 additions & 10 deletions sage_desktop_sdk/apis/cost_codes.py
Original file line number Diff line number Diff line change
Expand Up @@ -2,7 +2,6 @@
Sage Desktop Cost Codes
"""
from sage_desktop_sdk.core.client import Client
from sage_desktop_sdk.core.schema.read_only import CostCode


class CostCodes(Client):
Expand All @@ -20,16 +19,12 @@ def get_all_costcodes(self, version: int = None):
:return: A generator yielding cost codes in the Cost Code Schema
:rtype: generator of CostCode objects
"""
endpoint = CostCodes.GET_COST_CODE + '?page={0}'
if version:
# Append the version query parameter if provided
query_params = '?version={0}'.format(version)
endpoint = CostCodes.GET_COST_CODE + query_params
else:
endpoint = CostCodes.GET_COST_CODE
query_params = f'&version={version}'
endpoint += query_params

# Query the API to get all cost codes
cost_codes = self._query_get_all(endpoint)

for cost_code in cost_codes:
# Convert each cost code dictionary to a CostCode object and yield it
yield CostCode.from_dict(cost_code)
cost_codes = self._query_get_all_generator(endpoint, is_paginated=True)
yield cost_codes
Comment on lines +22 to +30
Copy link

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Ensure consistent use of string formatting for query parameters.

- endpoint = CostCodes.GET_COST_CODE + '?page={0}'
+ endpoint = CostCodes.GET_COST_CODE + f'?page={0}'

This change ensures that the string formatting is consistent with the rest of the code, which uses f-string formatting.


Committable suggestion

‼️ IMPORTANT
Carefully review the code before committing. Ensure that it accurately replaces the highlighted code, contains no missing lines, and has no issues with indentation.

Suggested change
endpoint = CostCodes.GET_COST_CODE + '?page={0}'
if version:
# Append the version query parameter if provided
query_params = '?version={0}'.format(version)
endpoint = CostCodes.GET_COST_CODE + query_params
else:
endpoint = CostCodes.GET_COST_CODE
query_params = f'&version={version}'
endpoint += query_params
# Query the API to get all cost codes
cost_codes = self._query_get_all(endpoint)
for cost_code in cost_codes:
# Convert each cost code dictionary to a CostCode object and yield it
yield CostCode.from_dict(cost_code)
cost_codes = self._query_get_all_generator(endpoint, is_paginated=True)
yield cost_codes
endpoint = CostCodes.GET_COST_CODE + f'?page={0}'
if version:
# Append the version query parameter if provided
query_params = f'&version={version}'
endpoint += query_params
# Query the API to get all cost codes
cost_codes = self._query_get_all_generator(endpoint, is_paginated=True)
yield cost_codes

Comment on lines +29 to +30
Copy link

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Yielding a generator object directly may not be the intended behavior.

- yield cost_codes
+ for cost_code in cost_codes:
+     yield cost_code

Directly yielding the generator object from _query_get_all_generator might not work as intended. Instead, iterate over the generator and yield each cost code individually.


Committable suggestion

‼️ IMPORTANT
Carefully review the code before committing. Ensure that it accurately replaces the highlighted code, contains no missing lines, and has no issues with indentation.

Suggested change
cost_codes = self._query_get_all_generator(endpoint, is_paginated=True)
yield cost_codes
cost_codes = self._query_get_all_generator(endpoint, is_paginated=True)
for cost_code in cost_codes:
yield cost_code

47 changes: 17 additions & 30 deletions sage_desktop_sdk/apis/jobs.py
Original file line number Diff line number Diff line change
Expand Up @@ -2,7 +2,6 @@
Sage Desktop Jobs
"""
from sage_desktop_sdk.core.client import Client
from sage_desktop_sdk.core.schema.read_only import Job, StandardCategory, StandardCostCode


class Jobs(Client):
Expand All @@ -22,19 +21,15 @@ def get_all_jobs(self, version: int = None):
:return: A generator yielding jobs in the Jobs Schema
:rtype: generator of Job objects
"""
endpoint = Jobs.GET_JOBS + '?page={0}'
if version:
# Append the version query parameter if provided
query_params = '?version={0}'.format(version)
endpoint = Jobs.GET_JOBS + query_params
else:
endpoint = Jobs.GET_JOBS
query_params = f'&version={version}'
endpoint += query_params

# Query the API to get all jobs
jobs = self._query_get_all(endpoint)

for job in jobs:
# Convert each job dictionary to a Job object and yield it
yield Job.from_dict(job)
jobs = self._query_get_all_generator(endpoint, is_paginated=True)
yield jobs
Comment on lines +24 to +32
Copy link

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Ensure consistent use of string formatting for query parameters.

- endpoint = Jobs.GET_JOBS + '?page={0}'
+ endpoint = Jobs.GET_JOBS + f'?page={0}'

This change ensures that the string formatting is consistent with the rest of the code, which uses f-string formatting.


Committable suggestion

‼️ IMPORTANT
Carefully review the code before committing. Ensure that it accurately replaces the highlighted code, contains no missing lines, and has no issues with indentation.

Suggested change
endpoint = Jobs.GET_JOBS + '?page={0}'
if version:
# Append the version query parameter if provided
query_params = '?version={0}'.format(version)
endpoint = Jobs.GET_JOBS + query_params
else:
endpoint = Jobs.GET_JOBS
query_params = f'&version={version}'
endpoint += query_params
# Query the API to get all jobs
jobs = self._query_get_all(endpoint)
for job in jobs:
# Convert each job dictionary to a Job object and yield it
yield Job.from_dict(job)
jobs = self._query_get_all_generator(endpoint, is_paginated=True)
yield jobs
endpoint = Jobs.GET_JOBS + f'?page={0}'
if version:
# Append the version query parameter if provided
query_params = f'&version={version}'
endpoint += query_params
# Query the API to get all jobs
jobs = self._query_get_all_generator(endpoint, is_paginated=True)
yield jobs

Comment on lines +31 to +32
Copy link

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Yielding a generator object directly may not be the intended behavior.

- yield jobs
+ for job in jobs:
+     yield job

Directly yielding the generator object from _query_get_all_generator might not work as intended. Instead, iterate over the generator and yield each job individually.


Committable suggestion

‼️ IMPORTANT
Carefully review the code before committing. Ensure that it accurately replaces the highlighted code, contains no missing lines, and has no issues with indentation.

Suggested change
jobs = self._query_get_all_generator(endpoint, is_paginated=True)
yield jobs
jobs = self._query_get_all_generator(endpoint, is_paginated=True)
for job in jobs:
yield job


def get_standard_costcodes(self, version: int = None):
"""
Expand All @@ -46,19 +41,15 @@ def get_standard_costcodes(self, version: int = None):
:return: A generator yielding standard cost codes in the Cost Code Schema
:rtype: generator of StandardCostCode objects
"""
endpoint = Jobs.GET_COST_CODES + '?page={0}'
if version:
# Append the version query parameter if provided
query_params = '?version={0}'.format(version)
endpoint = Jobs.GET_COST_CODES + query_params
else:
endpoint = Jobs.GET_COST_CODES

# Query the API to get all standard cost codes
costcodes = self._query_get_all(endpoint)
query_params = f'&version={version}'
endpoint += query_params

for costcode in costcodes:
# Convert each cost code dictionary to a StandardCostCode object and yield it
yield StandardCostCode.from_dict(costcode)
# Query the API to get all jobs
cost_codes = self._query_get_all_generator(endpoint, is_paginated=True)
yield cost_codes
Comment on lines +44 to +52
Copy link

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Ensure consistent use of string formatting for query parameters.

- endpoint = Jobs.GET_COST_CODES + '?page={0}'
+ endpoint = Jobs.GET_COST_CODES + f'?page={0}'

This change ensures that the string formatting is consistent with the rest of the code, which uses f-string formatting.


Committable suggestion

‼️ IMPORTANT
Carefully review the code before committing. Ensure that it accurately replaces the highlighted code, contains no missing lines, and has no issues with indentation.

Suggested change
endpoint = Jobs.GET_COST_CODES + '?page={0}'
if version:
# Append the version query parameter if provided
query_params = '?version={0}'.format(version)
endpoint = Jobs.GET_COST_CODES + query_params
else:
endpoint = Jobs.GET_COST_CODES
# Query the API to get all standard cost codes
costcodes = self._query_get_all(endpoint)
query_params = f'&version={version}'
endpoint += query_params
for costcode in costcodes:
# Convert each cost code dictionary to a StandardCostCode object and yield it
yield StandardCostCode.from_dict(costcode)
# Query the API to get all jobs
cost_codes = self._query_get_all_generator(endpoint, is_paginated=True)
yield cost_codes
endpoint = Jobs.GET_COST_CODES + f'?page={0}'
if version:
# Append the version query parameter if provided
query_params = f'&version={version}'
endpoint += query_params
# Query the API to get all jobs
cost_codes = self._query_get_all_generator(endpoint, is_paginated=True)
yield cost_codes

Comment on lines +51 to +52
Copy link

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Yielding a generator object directly may not be the intended behavior.

- yield cost_codes
+ for cost_code in cost_codes:
+     yield cost_code

Directly yielding the generator object from _query_get_all_generator might not work as intended. Instead, iterate over the generator and yield each cost code individually.


Committable suggestion

‼️ IMPORTANT
Carefully review the code before committing. Ensure that it accurately replaces the highlighted code, contains no missing lines, and has no issues with indentation.

Suggested change
cost_codes = self._query_get_all_generator(endpoint, is_paginated=True)
yield cost_codes
cost_codes = self._query_get_all_generator(endpoint, is_paginated=True)
for cost_code in cost_codes:
yield cost_code


def get_standard_categories(self, version: int = None):
"""
Expand All @@ -70,16 +61,12 @@ def get_standard_categories(self, version: int = None):
:return: A generator yielding standard categories in the Categories Schema
:rtype: generator of StandardCategory objects
"""
endpoint = Jobs.GET_CATEGORIES + '?page={0}'
if version:
# Append the version query parameter if provided
query_params = '?version={0}'.format(version)
endpoint = Jobs.GET_CATEGORIES + query_params
else:
endpoint = Jobs.GET_CATEGORIES
query_params = f'&version={version}'
endpoint += query_params

# Query the API to get all standard categories
categories = self._query_get_all(endpoint)

for category in categories:
# Convert each category dictionary to a StandardCategory object and yield it
yield StandardCategory.from_dict(category)
# Query the API to get all jobs
categories = self._query_get_all_generator(endpoint, is_paginated=True)
yield categories
Comment on lines +64 to +72
Copy link

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Ensure consistent use of string formatting for query parameters.

- endpoint = Jobs.GET_CATEGORIES + '?page={0}'
+ endpoint = Jobs.GET_CATEGORIES + f'?page={0}'

This change ensures that the string formatting is consistent with the rest of the code, which uses f-string formatting.


Committable suggestion

‼️ IMPORTANT
Carefully review the code before committing. Ensure that it accurately replaces the highlighted code, contains no missing lines, and has no issues with indentation.

Suggested change
endpoint = Jobs.GET_CATEGORIES + '?page={0}'
if version:
# Append the version query parameter if provided
query_params = '?version={0}'.format(version)
endpoint = Jobs.GET_CATEGORIES + query_params
else:
endpoint = Jobs.GET_CATEGORIES
query_params = f'&version={version}'
endpoint += query_params
# Query the API to get all standard categories
categories = self._query_get_all(endpoint)
for category in categories:
# Convert each category dictionary to a StandardCategory object and yield it
yield StandardCategory.from_dict(category)
# Query the API to get all jobs
categories = self._query_get_all_generator(endpoint, is_paginated=True)
yield categories
endpoint = Jobs.GET_CATEGORIES + f'?page={0}'
if version:
# Append the version query parameter if provided
query_params = f'&version={version}'
endpoint += query_params
# Query the API to get all jobs
categories = self._query_get_all_generator(endpoint, is_paginated=True)
yield categories

Comment on lines +71 to +72
Copy link

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Yielding a generator object directly may not be the intended behavior.

- yield categories
+ for category in categories:
+     yield category

Directly yielding the generator object from _query_get_all_generator might not work as intended. Instead, iterate over the generator and yield each category individually.


Committable suggestion

‼️ IMPORTANT
Carefully review the code before committing. Ensure that it accurately replaces the highlighted code, contains no missing lines, and has no issues with indentation.

Suggested change
categories = self._query_get_all_generator(endpoint, is_paginated=True)
yield categories
categories = self._query_get_all_generator(endpoint, is_paginated=True)
for category in categories:
yield category

25 changes: 8 additions & 17 deletions sage_desktop_sdk/apis/vendors.py
Original file line number Diff line number Diff line change
Expand Up @@ -2,7 +2,6 @@
Sage Desktop Vendors
"""
from sage_desktop_sdk.core.client import Client
from sage_desktop_sdk.core.schema.read_only import Vendor, VendorType


class Vendors(Client):
Expand All @@ -21,19 +20,15 @@ def get_all(self, version: int = None):
:return: A generator yielding vendors in the Vendors Schema
:rtype: generator of Vendor objects
"""
endpoint = Vendors.GET_VENDORS
if version:
# Append the version query parameter if provided
query_params = '?version={0}'.format(version)
endpoint = Vendors.GET_VENDORS + query_params
else:
endpoint = Vendors.GET_VENDORS
endpoint += query_params

# Query the API to get all vendors
vendors = self._query_get_all(endpoint)

for vendor in vendors:
# Convert each vendor dictionary to a Vendor object and yield it
yield Vendor.from_dict(vendor)
vendors = self._query_get_all_generator(endpoint)
yield vendors
Comment on lines +23 to +31
Copy link

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Ensure consistent use of string formatting for query parameters.

- query_params = '?version={0}'.format(version)
+ query_params = f'?version={version}'

This change ensures that the string formatting is consistent with the rest of the code, which uses f-string formatting.


Committable suggestion

‼️ IMPORTANT
Carefully review the code before committing. Ensure that it accurately replaces the highlighted code, contains no missing lines, and has no issues with indentation.

Suggested change
endpoint = Vendors.GET_VENDORS
if version:
# Append the version query parameter if provided
query_params = '?version={0}'.format(version)
endpoint = Vendors.GET_VENDORS + query_params
else:
endpoint = Vendors.GET_VENDORS
endpoint += query_params
# Query the API to get all vendors
vendors = self._query_get_all(endpoint)
for vendor in vendors:
# Convert each vendor dictionary to a Vendor object and yield it
yield Vendor.from_dict(vendor)
vendors = self._query_get_all_generator(endpoint)
yield vendors
endpoint = Vendors.GET_VENDORS
if version:
# Append the version query parameter if provided
query_params = f'?version={version}'
endpoint += query_params
# Query the API to get all vendors
vendors = self._query_get_all_generator(endpoint)
yield vendors

Comment on lines +30 to +31
Copy link

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Yielding a generator object directly may not be the intended behavior.

- yield vendors
+ for vendor in vendors:
+     yield vendor

Directly yielding the generator object from _query_get_all_generator might not work as intended. Instead, iterate over the generator and yield each vendor individually.


Committable suggestion

‼️ IMPORTANT
Carefully review the code before committing. Ensure that it accurately replaces the highlighted code, contains no missing lines, and has no issues with indentation.

Suggested change
vendors = self._query_get_all_generator(endpoint)
yield vendors
vendors = self._query_get_all_generator(endpoint)
for vendor in vendors:
yield vendor


def get_vendor_types(self, version: int = None):
"""
Expand All @@ -45,16 +40,12 @@ def get_vendor_types(self, version: int = None):
:return: A generator yielding vendor types in the Vendor Types Schema
:rtype: generator of VendorType objects
"""
endpoint = Vendors.GET_VENDOR_TYPES
if version:
# Append the version query parameter if provided
query_params = '?version={0}'.format(version)
endpoint = Vendors.GET_VENDOR_TYPES + query_params
else:
endpoint = Vendors.GET_VENDOR_TYPES
endpoint += query_params

# Query the API to get all vendor types
vendor_types = self._query_get_all(endpoint)

for vendor_type in vendor_types:
# Convert each vendor type dictionary to a VendorType object and yield it
yield VendorType.from_dict(vendor_type)
vendor_types = self._query_get_all_generator(endpoint)
yield vendor_types
Comment on lines +43 to +51
Copy link

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Ensure consistent use of string formatting for query parameters.

- query_params = '?version={0}'.format(version)
+ query_params = f'?version={version}'

This change ensures that the string formatting is consistent with the rest of the code, which uses f-string formatting.


Committable suggestion

‼️ IMPORTANT
Carefully review the code before committing. Ensure that it accurately replaces the highlighted code, contains no missing lines, and has no issues with indentation.

Suggested change
endpoint = Vendors.GET_VENDOR_TYPES
if version:
# Append the version query parameter if provided
query_params = '?version={0}'.format(version)
endpoint = Vendors.GET_VENDOR_TYPES + query_params
else:
endpoint = Vendors.GET_VENDOR_TYPES
endpoint += query_params
# Query the API to get all vendor types
vendor_types = self._query_get_all(endpoint)
for vendor_type in vendor_types:
# Convert each vendor type dictionary to a VendorType object and yield it
yield VendorType.from_dict(vendor_type)
vendor_types = self._query_get_all_generator(endpoint)
yield vendor_types
endpoint = Vendors.GET_VENDOR_TYPES
if version:
# Append the version query parameter if provided
query_params = f'?version={version}'
endpoint += query_params
# Query the API to get all vendor types
vendor_types = self._query_get_all_generator(endpoint)
yield vendor_types

Comment on lines +50 to +51
Copy link

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Yielding a generator object directly may not be the intended behavior.

- yield vendor_types
+ for vendor_type in vendor_types:
+     yield vendor_type

Directly yielding the generator object from _query_get_all_generator might not work as intended. Instead, iterate over the generator and yield each vendor type individually.


Committable suggestion

‼️ IMPORTANT
Carefully review the code before committing. Ensure that it accurately replaces the highlighted code, contains no missing lines, and has no issues with indentation.

Suggested change
vendor_types = self._query_get_all_generator(endpoint)
yield vendor_types
vendor_types = self._query_get_all_generator(endpoint)
for vendor_type in vendor_types:
yield vendor_type

Loading
Loading