Skip to content

Add compatibility for DLCs#10

Open
shaunikm wants to merge 6 commits into
gadhagod:masterfrom
shaunikm:master
Open

Add compatibility for DLCs#10
shaunikm wants to merge 6 commits into
gadhagod:masterfrom
shaunikm:master

Conversation

@shaunikm
Copy link
Copy Markdown
Collaborator

@shaunikm shaunikm closed this Jun 28, 2022
@shaunikm shaunikm reopened this Jun 28, 2022
@shaunikm
Copy link
Copy Markdown
Collaborator Author

Fixed syntax issue

Copy link
Copy Markdown
Owner

@gadhagod gadhagod left a comment

Choose a reason for hiding this comment

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

Thanks for your work @shaunikm. Just added a few comments. Please address them before I merge.

Comment thread pyrule_compendium/__init__.py
Comment thread pyrule_compendium/__init__.py Outdated
"""

def __init__(self, base_url: str="https://botw-compendium.herokuapp.com/api/v2", default_timeout: Union[int, float, None]=None):
def __init__(self, base_url: str="https://botw-compendium.herokuapp.com/api/v2", default_timeout: Union[int, float, None]=None, master_mode: bool = False):
Copy link
Copy Markdown
Owner

Choose a reason for hiding this comment

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

I think master_mode should be passed into the functions rather than to the class, because we do not want to create another object for master_mode data.

Copy link
Copy Markdown
Collaborator Author

Choose a reason for hiding this comment

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

I feel that having master_mode passed when the class is initialized would remove the need for a redundant parameter in every function of the class.

Comment thread pyrule_compendium/__init__.py Outdated
Comment thread pyrule_compendium/__init__.py
Comment thread pyrule_compendium/__init__.py Outdated
if not timeout:
timeout = self.default_timeout

res: dict = self.api.request(f"/entry/{entry}", timeout)
Copy link
Copy Markdown
Owner

Choose a reason for hiding this comment

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

I plan on adding a dlc field to entry data in v3. Until then, is there a better way of determining if it is master_mode or not without two requests?

Copy link
Copy Markdown
Collaborator Author

Choose a reason for hiding this comment

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

Other than using local data, I don’t see any way to determine if an entry is from master_mode without two requests.

res: dict = self.api.request(f"/entry/{entry}", timeout)
if res: return False

res = self.master_api.request(f"/entry/{entry}", timeout)
Copy link
Copy Markdown
Owner

Choose a reason for hiding this comment

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

Too many requests?

Copy link
Copy Markdown
Collaborator Author

Choose a reason for hiding this comment

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

Again, I don’t see a way to minimize the number of requests we have here other than using local data.

shaunikm added 3 commits June 28, 2022 16:57
- Can also override class parameter `master_mode`
Reduce number of requests when calling function
@shaunikm shaunikm added the enhancement New feature or request label Jun 29, 2022
@shaunikm shaunikm changed the title Add compatibility for master mode Add compatibility for DLCs Jul 16, 2022
@gadhagod gadhagod self-requested a review September 4, 2022 18:14
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Labels

enhancement New feature or request

Projects

None yet

Development

Successfully merging this pull request may close these issues.

2 participants