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

Expand some userpaths in internals.py #2825 #3128

Open
wants to merge 3 commits into
base: develop
Choose a base branch
from

Conversation

ethanknights
Copy link

@ethanknights ethanknights commented Mar 4, 2023

Hi - I added some of the path expansions to internals.py as described in #2825.

The tox command seems ok (output below).

================= 488 passed, 34 skipped, 9 xfailed, 1 warning in 249.04s (0:04:09) ==================
.pkg: _exit nltk/test> python /Users/ethanknights/PycharmProjects/nltk/venv/lib/python3.9/site-packages/pyproject_api/_backend.py True setuptools.build_meta __legacy__
  python3.7: OK (273.95=setup[17.25]+cmd[1.94,254.76] seconds)
  congratulations :) (274.45 seconds)

Note - I tried expanding the remaining paths with a few different approaches. e.g., Adding a utility function that converted tuples to lists and expanded each of their items, like this:

file_names = expand_list_of_userpaths(file_names)
searchpath = expand_list_of_userpaths(searchpath)

But this wasn't a robust solution (particularly for the filename variable), so these paths remain unchanged for now.
In any-case, hopefully this PR is a start!

Copy link
Member

@tomaarsen tomaarsen left a comment

Choose a reason for hiding this comment

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

Other than the one comment I made, the changes seem reasonable, but do you know if it would be possible to make tests for them? This may not be feasible (as the goal is to have the tests pass locally and also on the CI), but it may be worthwhile.
For example, having an environment variable that points to "~\test_dir", which is then used by find_file (or find_file, find_binary, find_jar_iter, etc.):

def find_file(

Perhaps this is possible by creating a temporary file/folder for a test? Maybe there's some permission issues with that, though...

@@ -540,6 +540,7 @@ def find_file_iter(

# Check environment variables
for env_var in env_vars:
env_var = os.path.expanduser(env_var)
Copy link
Member

Choose a reason for hiding this comment

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

env_var is just a string like "HUNPOS_TAGGER", e.g.:

path_to_model, env_vars=("HUNPOS_TAGGER",), verbose=verbose

env_vars=("STANFORD_MODELS",),

return find_file(model_filename, env_vars=("MALT_MODEL",), verbose=False)

So, we don't want to expand these.

Copy link
Author

Choose a reason for hiding this comment

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

Got it, thanks. Reverted this change in 9797986.

@ethanknights
Copy link
Author

Hey @tomaarsen - apologies for the piecemeal approach.

I've made the requested change in 9797986. Feel free to review/mark as resolved.

As for the tests - I agree with the sentiment but am conscious of creating files in a users' home directory (e.g., we need to ensure we are creating/deleting an obscure directory/filename (or within nltk-data/ etc.) as to not interfere with user setups).

Happy to come back to adding the doctest, after more thought - I can create an issue about this now?

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Projects
None yet
Development

Successfully merging this pull request may close these issues.

None yet

2 participants