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

fixed the nltk hook and hook for wavefile #3775

Closed
wants to merge 14 commits into from
Closed

fixed the nltk hook and hook for wavefile #3775

wants to merge 14 commits into from

Conversation

brightening-eyes
Copy link
Contributor

hi,
i was working with some audio filetypes and nltk, and i ran into some problem regarding libsndfile-1.dll
so, i've made this hook for wavefile
also fixed the loader for nltk witch had 2 underscores instead of 1 on its filename

Copy link
Member

@htgoebel htgoebel left a comment

Choose a reason for hiding this comment

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

Thanks for this pull-request.

Please clean it put so I can merge it. Since this are actually two new hooks, it should end up into two commits. And the commit messages should be like "Hooks: Add hook for ..." - since these are no "fixes" but new hooks. You will also need two news fragments (being part of the respective commit).

Also there are still lint-errors:

  Found violations: PyInstaller/hooks/hook-wavefile.py
	E501 @ 11:80 - line too long (83 > 79 characters)

# add datas for nltk
datas = collect_data_files('nltk', False)

# loop through the data directories and add them
Copy link
Member

Choose a reason for hiding this comment

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

Can you pleae be a bit more verbose here. What is the nltk.data.path? Why is it okay to put all data into a single directory?

Choose a reason for hiding this comment

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

nltk.data.path is an environment variable created by nltk while importing it. I have tested this hook file however, it needs some modification to run without any errors.

@htgoebel htgoebel added area:hooks Caused by or effecting some hook pr:commit-message commit message needs revision pr:changelog-entry Changelog entry missing or needs revision labels Oct 2, 2018
@htgoebel
Copy link
Member

htgoebel commented Oct 2, 2018

I forgot:
When updating a pull-request, you can simply (force) push the updated branch to github again. This will automatically update the pull-request (which follows the branch, not the commit). So you do not need to close the pull-request and open a new one. This also has the benefit that the discussion history is kept. For detailed instructions please read Updating a Pull-Request in the manual..

@brightening-eyes
Copy link
Contributor Author

about nltk.data.path, this is my only way of doing this. if you know a better solution, let me know
the data files in these directories should be placed somewhere and then nltk should be able to load it.

@htgoebel
Copy link
Member

htgoebel commented Oct 4, 2018

I don't use nltk, thus I can not give you any tip. Please add a comment so somebody with no knowledge about nltk can understand what nltk.data.path is and why it is save to put all data into one directory.

Please mind cleanup, adding changelog-entries and splitting into two commits. Thanks.

@htgoebel
Copy link
Member

@brightening-eyes Pinging. Please finish this pull-request so I can merge it. Please also rebase, since there are conflicts. Thanks.

@Legorooj
Copy link
Member

Stale

@Legorooj Legorooj closed this Feb 16, 2020
@github-actions github-actions bot locked as resolved and limited conversation to collaborators Nov 16, 2022
Sign up for free to subscribe to this conversation on GitHub. Already have an account? Sign in.
Labels
area:hooks Caused by or effecting some hook pr:changelog-entry Changelog entry missing or needs revision pr:commit-message commit message needs revision state:needs more work This PR needs more work
Projects
None yet
Development

Successfully merging this pull request may close these issues.

None yet

4 participants