Skip to content

Conversation

Christiandus
Copy link

@Christiandus Christiandus commented Sep 5, 2025

Hi

I did some cleanup of the code:

  • Typos
  • Better use of static variables

Implemented more robustness:

  • Use urlunparse and urljoin to more robustly join URLs and avoid f-strings

Some logic improvement:

  • simplify port logic (remove exceptions for port 80 and 443 and treat them the same as any other port)
  • Session does not need to be deleted, if it is set to None after
  • Remove valid_url function as it is made unnecessary by the proper use of urljoin
  • Implement hostname checking (ipv4, ivp6, rfc-compliant)

Test adjustments:

  • Remove tests for valid_url (function is removed)
  • Implement tests for hostname checking

Let me know what you think

@vladimirs-git
Copy link
Owner

Thanks for your interest and suggestions!
The latest improvements are in v2.0.7.

  • Replaced f-strings for URLs with helpers.py url_join() to make URL building more robust. I chose not to use urllib.parse.urljoin because it behaves differently when the base URL ends with a /.
  • Agreed with the simplified port handling logic. All ports, including 80 and 443, are now treated uniformly.
  • Improved hostname validation for IPv6, but I don’t fully agree with enforcing strict RFC-compliant hostnames. Since this project focuses on the API, I don’t see a strong reason to perfectly validate user input in this context.

Let me know if you have any additional feedback or if there’s anything else you’d like me to adjust.

@Christiandus
Copy link
Author

Thank you!
I see all your points. In the fortigate_base.py file however, the urljoin has not been replaced with the helper function from the helpers.py file. I think this might be good, to have improved consistency.

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.

2 participants