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

API method to change the name of a creative tonie #58

Open
wants to merge 5 commits into
base: main
Choose a base branch
from

Conversation

TomFreudenberg
Copy link

Hi and thanks for the component!

I like to contribute if you are interested.

The patch allows to change the name of a creative tonie in your households:

api.change_name_of_tonie(tonie, "new name")

Cheers
Tom

@Wilhelmsson177
Copy link
Owner

Hello Tom,

thank you for your request. The Idea is good and I'd like to merge it. Before, I do I just want check some parts.

The current change would break the API, since you have introduced a new user_agent argument. Can you explain the reason for this? You could set a default value in the init method and it would be fine.
Additionally, the order of the methods seems to be messed up a bit and therefore the change is hard to review. Can you leave the order like it was before? For a rearrangement, we could add a new merge request, if there really is a necessity.

I just approved to execute the unit tests and they all failed. Can you update the unit tests as well and add a test for your method.

Additionally, there are some issues with the commit messages they get checked to follow the conventional commit message style, but no worries, I will get them fixed doing a squashed merge.

Thank you for your participation.

Willi

@TomFreudenberg
Copy link
Author

TomFreudenberg commented Feb 12, 2024

Dear Willi,

thanks for your comments and feedback.

I am sorry, that my commits disturbed this PR - I hadn't that in mind!

I started this with my first commit only and that was the reason for this PR.

Maybe the second and third commit is also interesting for the API in general.

The last changes I made are resulting while we have created a service on Tonies API and had some problems when uploading stuff to tonies the way we have to do it.

So I decided to "be incompatible" by my fork but can handle our requirements in special.

So how do your like to proceed? I am open in everything!

Cheers
Tom

Copy link
Owner

@Wilhelmsson177 Wilhelmsson177 left a comment

Choose a reason for hiding this comment

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

Can you make the changes and make sure the tests run. I will then merge it and make a new release with mentioning the braeking changes.

@@ -121,7 +121,7 @@ def get_creative_tonie(self, creative_tonie: CreativeTonie) -> CreativeTonie:
"""
url = f"households/{creative_tonie.householdId}/creativetonies/{creative_tonie.id}"
ct = self._get(url=url)
return CreativeTonie(**ct)
return CreativeTonie(**ct) if ct else ct
Copy link
Owner

Choose a reason for hiding this comment

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

I don't think returning the response is a good idea in this case. Shouldn't it be

response = self._get(url=url)
return CreativeTonie(**response) if response else None

The code is untested, just my understanding.

Comment on lines +32 to +37
def __init__(self, username: str, password: str, user_agent: str) -> None:
"""Initializes the API and creates a session token for tonie cloud session."""
if not user_agent or user_agent == "":
self.user_agent = "tonieApi/2.0"
else:
self.user_agent = user_agent
Copy link
Owner

Choose a reason for hiding this comment

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

I haven't found anything in the api regarding the User-Agent Header. But I also don't see anything which should block it.

 def __init__(self, username: str, password: str, user_agent: str = "tonieApi/2.0") -> None:
        """Initializes the API and creates a session token for tonie cloud session."""
        self.user_agent = user_agent

Should also fix the tests.

Copy link
Author

Choose a reason for hiding this comment

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

Hi @Wilhelmsson177 Willi

I prepare an update and tests but please allow me some time to do.

Tom

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

None yet

2 participants