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

Add module for the switching endpoint of the API: #112

Merged
merged 1 commit into from
May 11, 2022

Conversation

earendilfr
Copy link

@WhaleJ84
Copy link
Owner

WhaleJ84 commented May 9, 2022

Finished reviewing; works perfectly fine on my end. All the recommendations above are minor and I can do myself but I thought I'd note them down in case you want to resolve them yourself. Will merge in if no reply is given after 3 days. Thanks for adding!

@WhaleJ84
Copy link
Owner

WhaleJ84 commented May 9, 2022

Moving to own comment, as it looks like I'm calling it an issue in the code

I appreciate the addition of the docstring entry on line 55! I should probably make note of these documentation inconsistencies and submit issues in the official repo

@earendilfr
Copy link
Author

Ok: I have updated the PR with the recommendation provide by pylint.

Just one question: for your second remarks: did you prefer I replace

      def get_link(self, link: int):
      ¦   """
      ¦   Retrieves Link by ID
  
      ¦   :param link: Should be a link ID (integer)
      ¦   """

by

      def get_link(self, link: str):
      ¦   """
      ¦   Retrieves Link by ID
  
      ¦   :param link: Should be a link ID
      ¦   """

@WhaleJ84
Copy link
Owner

Ok: I have updated the PR with the recommendation provide by pylint.

Just one question: for your second remarks: did you prefer I replace

      def get_link(self, link: int):
      ¦   """
      ¦   Retrieves Link by ID
  
      ¦   :param link: Should be a link ID (integer)
      ¦   """

by

      def get_link(self, link: str):
      ¦   """
      ¦   Retrieves Link by ID
  
      ¦   :param link: Should be a link ID
      ¦   """

I tested both strings and integers and they both work. In my experience, the LibreNMS API tends to prefer string interpretations of integers, so I would go with strings. You could always do a Union instead and accept both.

@earendilfr earendilfr force-pushed the main branch 2 times, most recently from f1fe50a to f6cf798 Compare May 10, 2022 19:52
@earendilfr
Copy link
Author

Ok: I have updated the PR with the recommendation provide by pylint.
Just one question: for your second remarks: did you prefer I replace

      def get_link(self, link: int):
      ¦   """
      ¦   Retrieves Link by ID
  
      ¦   :param link: Should be a link ID (integer)
      ¦   """

by

      def get_link(self, link: str):
      ¦   """
      ¦   Retrieves Link by ID
  
      ¦   :param link: Should be a link ID
      ¦   """

I tested both strings and integers and they both work. In my experience, the LibreNMS API tends to prefer string interpretations of integers, so I would go with strings. You could always do a Union instead and accept both.

OK: so, i have updated the PR and now use str as parameter.

@WhaleJ84 WhaleJ84 merged commit 0f2f035 into WhaleJ84:main May 11, 2022
This was referenced May 11, 2022
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