Small changes #46
Small changes #46
Conversation
|
Ugh. I didn't realize my further changes would be "added" to my original PR. That said, it appears my code works up to this point. However, I think we should discuss some of the later changes I'm working on... such as adding the 'search' option in the Iterator and the new admin_and_role_management.py |
| @@ -4,5 +4,5 @@ | |||
| "Dax Mickelson", | |||
| ] | |||
|
|
|||
| from zia import ZIA | |||
| from zpa import ZPA | |||
| from .zia import ZIA | |||
There was a problem hiding this comment.
"dot" was needed to successfully run in PyCharm. Not sure why it is failing here though.
| from zia import ZIA | ||
| from zpa import ZPA | ||
| from .zia import ZIA | ||
| from .zpa import ZPA |
There was a problem hiding this comment.
"dot" was needed to successfully run in PyCharm. Not sure why it is failing here though.
.gitignore
Outdated
| @@ -0,0 +1,140 @@ | |||
| .idea | |||
There was a problem hiding this comment.
We should have a .gitignore file, totally missed adding one!
I'd prefer it's specific to this project though to keep it concise (we don't use Django, Flask etc). This stuff is better left to people's own environments.
There was a problem hiding this comment.
It's your project so let's implement it how you'd like it. That said, I like to error on the side of missing something being imported into my projects vs having something in my git history for time and all eternity. :-) (Like MAC .DS_STORE or Windows desktop.ini files.)
If/when you make a .gitignore I'd suggest you add those two in. :-)
There was a problem hiding this comment.
I've been using a global ignore (see here for a quick primer) and it's working great. Check it out if you haven't used it before as it will ensure you get the same feel across every project you work with.
There was a problem hiding this comment.
That is great for you, the user on your machine, but what about anyone who pushes code to this repo? Won't you end up with .DS_STORE, etc. all the time?
It's your repo and we will work with it as you wish. I'm just trying to fully understand how the global .gitignore would work in this use case.
There was a problem hiding this comment.
Agree that it's not going to stop any random person. But I see that as part of the pull request and review process. I'm not going to approve any changes that include unwanted files. I will say that if this got to be such an issue that it was taking me hours because everyone was doing it then sure I'm happy to revisit that decision.
My overarching philosophy is this:
- I personally don't believe any contributors should rely on a project
.gitignorebeing complete and providing safety for every OS/tooling combination possible. - There are no other maintainers so all contributed changes need to occur via PR.
- Every time a new editor, tool etc becomes popular then there would be PRs to add the miscellany created by them to
.gitignore - Unwanted files only end up in a commit because a developer has performed
git add .without having protections (globalcore.excludesfile) in place - It takes me a few seconds to spot unwanted files in 'Files changed' under the PR
- It takes a few seconds more to nudge the contributor to address these with their
core.excludesfile, solving the problem for future PRs from that contributor - If it's an artefact created by pyZscaler during run, build or test that's unwanted and has been omitted then we will accept a PR to updated the
.gitignore
The more I've thought about this, the more I'm happy with this approach. Sorry if this is going to cause any issues contributing.
There was a problem hiding this comment.
No issues on my side! Just stating my point of view for your review. :-)
pyzscaler/utils.py
Outdated
| @@ -50,10 +50,14 @@ def __init__(self, api, path="", **kw): | |||
|
|
|||
| self.path = path | |||
|
|
|||
| search = '' | |||
There was a problem hiding this comment.
Glad you found this as well, this is covered by #42.
There was a problem hiding this comment.
Sorry to be impatient but any ETA when this will be committed/merged and updated in pypi? I REALLY need this feature. :-)
There was a problem hiding this comment.
Do you mind just dropping the changes to utils.py seeing as #42 has merged into develop? Tests should pass after that and then I'll approve and merge this one in.
I've recently changed PRs to be against develop, that way I can bundle changes into releases easily via Gitflow.
| @@ -0,0 +1,30 @@ | |||
| from restfly.endpoint import APIEndpoint | |||
| @@ -17,6 +17,7 @@ | |||
| from .url_filters import URLFilteringAPI | |||
There was a problem hiding this comment.
IMHO, I would prefer we use absolute imports eg: pyzscaler.zia.url_filters. I think its more clear, but that's more of a personal preference.
| @@ -52,7 +52,7 @@ from pyzscaler.zia import ZIA | |||
| from pprint import pprint | |||
|
|
|||
| zia = ZIA(api_key='API_KEY', cloud='CLOUD', username='USERNAME', password='PASSWORD') | |||
| for user in zia.users.list(): | |||
| for user in zia.users.list_users(): | |||
There was a problem hiding this comment.
Ahhh great find, can't believe that sat there for that long!
|
Approved. Merging. |
Added a contributors variable.
Refactored the layout of one method's variables.
Exposed ZIA and ZPA Classes to top level module. (You can now
from pyzscaler import ZIA.)Alas, I don't know how to run your tests but I did open a python console and made sure the import statements worked.