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 --targets to CLI #335

Merged
merged 8 commits into from
Mar 6, 2018
Merged

Add --targets to CLI #335

merged 8 commits into from
Mar 6, 2018

Conversation

aardschok
Copy link
Contributor

Solves #334

  • Added --targets through publish
  • Added check for PYBLISH_TARGETS env variable
    if targets are found register them with the API
  • Minor cosmetics

@tokejepsen
Copy link
Member

Looks good to me. Maybe we need some tests for this?

@aardschok
Copy link
Contributor Author

I'll setup a test for targets

@tokejepsen
Copy link
Member

Looks good to me. @mottosso ?

pyblish/api.py Outdated
@@ -151,6 +151,11 @@ def __init__():
if host:
register_host(host)

# Register targets for current session
for t in os.environ.get("PYBLISH_TARGETS", "").split(os.pathsep):
Copy link
Member

Choose a reason for hiding this comment

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

This is too short of a variable name.

Can we also make it..

if not target:
  continue

register_target(target)

To reduce indentation?

Copy link
Member

Choose a reason for hiding this comment

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

Alternatively..

register_target(target) if target else None

Copy link
Contributor Author

Choose a reason for hiding this comment

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

If I do that then I want to change the lines (line 150) above it as well to have consistency.
I used this as a template to ensure the logic would be the same:

# Register hosts from environment "PYBLISHHOSTS"
for host in os.environ.get("PYBLISH_HOSTS", "").split(os.pathsep):
    if host:
        register_host(host)

These lines would then become:

# Register hosts from environment "PYBLISHHOSTS"
for host in os.environ.get("PYBLISH_HOSTS", "").split(os.pathsep):
    if not host:
        continue
    register_host(host)

Agreed?

@mottosso mottosso changed the title Improve CLI Add --targets to CLI Mar 5, 2018
@mottosso
Copy link
Member

mottosso commented Mar 5, 2018

Ok, with that minor tweak I'm happy.

@mottosso mottosso merged commit 841f408 into pyblish:master Mar 6, 2018
@mottosso
Copy link
Member

mottosso commented Mar 6, 2018

Thanks!

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.

3 participants