Skip to content

skip directories with perm error when building autoimport index - #733

Merged
lieryan merged 11 commits into
python-rope:masterfrom
MrBago:permError
Jan 3, 2024
Merged

lieryan merged 11 commits into
python-rope:masterfrom
MrBago:permError

Conversation

@MrBago

@MrBago MrBago commented Dec 1, 2023

Copy link
Copy Markdown

Description

WIth this fix, rope will skip folders in sys.path that the user does not have permission to access instead of failing the entire cache build.

Fixes #(issue)

Checklist (delete if not relevant):

  • I have added tests that prove my fix is effective or that my feature works
  • I have updated CHANGELOG.md
  • I have made corresponding changes to user documentation for new features
  • I have made corresponding changes to library documentation for API changes

@MrBago

MrBago commented Dec 1, 2023

Copy link
Copy Markdown
Author

I ran into this minor issue when working on my other PR so here's a quick patch.

@tkrabel-db

Copy link
Copy Markdown

@MrBago thanks for that! LGTM.

Comment thread rope/contrib/autoimport/sqlite.py Outdated
Comment thread ropetest/contrib/autoimporttest.py Outdated
with tempfile.TemporaryDirectory() as dir:
import os
os.chmod(dir, 0)
sys.path.append(dir)

Copy link
Copy Markdown
Member

Choose a reason for hiding this comment

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

This test lacks isolation, changing the sys.path here is global mutation that won't be undone by the end of the test so it will affect other tests as well.

Copy link
Copy Markdown

Choose a reason for hiding this comment

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

@MrBago I think it's enough to remove the dir at the end of the context manager.

Copy link
Copy Markdown
Author

Choose a reason for hiding this comment

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

@lieryan I updated the test to use prefs.python_paths instead of sys.path.

Comment thread ropetest/contrib/autoimporttest.py Outdated

@lieryan lieryan left a comment

Copy link
Copy Markdown
Member

Choose a reason for hiding this comment

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

Tests shouldn't be changing the sys.path globally

Comment thread ropetest/contrib/autoimporttest.py Outdated
Comment thread ropetest/contrib/autoimporttest.py
@MrBago
MrBago requested a review from lieryan December 21, 2023 19:33
@MrBago

MrBago commented Jan 2, 2024

Copy link
Copy Markdown
Author

@lieryan Happy new year! When you have a min, can you look the update I made to this PR?

@lieryan lieryan changed the title skip sys.path dirs with perm error skip directories with perm error when building autoimport index Jan 3, 2024
@lieryan
lieryan enabled auto-merge January 3, 2024 01:08
@lieryan

lieryan commented Jan 3, 2024

Copy link
Copy Markdown
Member

Thanks for fixing this issue @MrBago

@lieryan

lieryan commented Jan 3, 2024

Copy link
Copy Markdown
Member

@all-contributors add @MrBago for code contribution

@allcontributors

Copy link
Copy Markdown
Contributor

@lieryan

I've put up a pull request to add @MrBago! 🎉

@lieryan
lieryan merged commit 174f084 into python-rope:master Jan 3, 2024
@lieryan lieryan added this to the 1.12.0 milestone Jan 18, 2024
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