Skip to content

Conversation

@kevin-thankyou-lin
Copy link
Contributor

What this does

Allow imports to continue but log error. Exiting program on error means the docs can't build.

Copy link
Member

@Abhiram824 Abhiram824 left a comment

Choose a reason for hiding this comment

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

I see what you are doing here, but outside of fixing the docs, it may not make sense to put pynput import in a try-catch?

Perhaps we should just localize this change to the ci-docs branch? Or maybe it isnt that big of a deal. What do you think?

Copy link
Contributor

@abhihjoshi abhihjoshi left a comment

Choose a reason for hiding this comment

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

Yeah I agree that putting it in a try block might not be ideal. Not sure how much sense this makes, but maybe what we can do is set an env variable prior to building the docs. Then, in the code, we can add some check that says if BUILD_DOCS=true (or something of that nature), ignore importing pynput (and other packages causing issues with the docs).

@kevin-thankyou-lin
Copy link
Contributor Author

I see what you are doing here, but outside of fixing the docs, it may not make sense to put pynput import in a try-catch?

Perhaps we should just localize this change to the ci-docs branch? Or maybe it isnt that big of a deal. What do you think?

Yeah, ideally the program exits, so this was the choice I landed on for telling the user that the import won't work. I think localizing it to ci-docs is tough because the website gets rebuilt on any update to master.

Yeah I agree that putting it in a try block might not be ideal. Not sure how much sense this makes, but maybe what we can do is set an env variable prior to building the docs. Then, in the code, we can add some check that says if BUILD_DOCS=true (or something of that nature), ignore importing pynput (and other packages causing issues with the docs).

Do you mean something like this?

# Check the environment variable
if os.getenv("BUILD_DOCS") != "true":
    # Import pynput only if BUILD_DOCS is not set to true
    from pynput import keyboard

@abhihjoshi
Copy link
Contributor

Yeah I agree that putting it in a try block might not be ideal. Not sure how much sense this makes, but maybe what we can do is set an env variable prior to building the docs. Then, in the code, we can add some check that says if BUILD_DOCS=true (or something of that nature), ignore importing pynput (and other packages causing issues with the docs).

Do you mean something like this?

# Check the environment variable
if os.getenv("BUILD_DOCS") != "true":
    # Import pynput only if BUILD_DOCS is not set to true
    from pynput import keyboard

Yeah, like that. I think we can add the BUILD_DOCS env var in the workflow.

@Abhiram824
Copy link
Member

I see what you are doing here, but outside of fixing the docs, it may not make sense to put pynput import in a try-catch?
Perhaps we should just localize this change to the ci-docs branch? Or maybe it isnt that big of a deal. What do you think?

Yeah, ideally the program exits, so this was the choice I landed on for telling the user that the import won't work. I think localizing it to ci-docs is tough because the website gets rebuilt on any update to master.

Yeah I agree that putting it in a try block might not be ideal. Not sure how much sense this makes, but maybe what we can do is set an env variable prior to building the docs. Then, in the code, we can add some check that says if BUILD_DOCS=true (or something of that nature), ignore importing pynput (and other packages causing issues with the docs).

Do you mean something like this?

# Check the environment variable
if os.getenv("BUILD_DOCS") != "true":
    # Import pynput only if BUILD_DOCS is not set to true
    from pynput import keyboard

yeah I think this makes the most sense

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.

4 participants