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

Fix device docs - allow imports to continue but log error #614

Open
wants to merge 1 commit into
base: master
Choose a base branch
from

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.

3 participants