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

add speech-tagger recipe #3053

Merged
merged 3 commits into from
Sep 3, 2015
Merged

add speech-tagger recipe #3053

merged 3 commits into from
Sep 3, 2015

Conversation

cosmicexplorer
Copy link
Contributor

speech-tagger is an extension to tag parts of speech in an emacs buffer
using stanford coreNLP. It can be found at
https://github.com/cosmicexplorer/speech-tagger. I am the maintainer of
this package.

speech-tagger is an extension to tag parts of speech in an emacs buffer
using stanford coreNLP. It can be found at
https://github.com/cosmicexplorer/speech-tagger. I am the maintainer of
this package.
@purcell
Copy link
Member

purcell commented Aug 27, 2015

Thanks, looks good. I'd strongly encourage you to follow the elisp coding conventions for symbol naming, though, because the clojure-style naming here is not at all idiomatic.

Additionally, triple-semicolon comments have a specific meaning, so only use them for standard section headings, e.g. Commentary:, Code:, Usage:, plus the first and last comment lines.

Hope that helps,

-Steve

@purcell purcell added recipes awaiting-upstream Awaiting action from an upstream maintainer labels Aug 27, 2015
@cosmicexplorer
Copy link
Contributor Author

Fixed the clojure naming and kept triple-semicolon lines to bookends only. Thanks for the link to those conventions.

I've also made a change (which you can see in this pull request) to the packaging; instead of downloading the jar at runtime, it pulls it in as part of the package, which hopefully takes fewer people by surprise.

@cosmicexplorer
Copy link
Contributor Author

Just fixed an issue where the "loading" overlay wasn't being removed by the clear-tags function, and made the jar path a defcustom to facilitate easier development.

@purcell
Copy link
Member

purcell commented Aug 31, 2015

Sorry for the slow feedback. The .el file looks super now, but we'd strongly prefer to build from your dev branch, not a build branch that is manually updated and may one day get stale without anybody noticing. So to that extent, it would be preferable for the code to come from master, and for the package to download the jar at runtime (or, probably, load time).

@cosmicexplorer
Copy link
Contributor Author

Alright, that's what I had before. I'm not familiar with eval-when but I'll make it check the jar path for existence and download if not. That does make versioning the jar a tad more difficult, I'll try to see if I can quickly add a hashing scheme to detect versioning without too much hackery.

@cosmicexplorer
Copy link
Contributor Author

Ok, it's proving harder than I thought to download a file over http; url-retrieve-synchronously is either dropping bytes, or emacs is converting the coding system incorrectly or something, but I'm still working on this. I could make every speech tag command shoot a network request to my own server, and then I wouldn't have to deal with this at all, but the latency would be super annoying. I'll comment again when I can make this work. Thanks a lot for your help.

@cosmicexplorer
Copy link
Contributor Author

Ok, turned out the coding system was the problem; specifying 'raw-text as the coding system in the buffer returned by url-retrieve-synchronously seems to have resolved the hashing issues. I've run this through a few tests on an empty emacs and it seems to work just fine.

purcell added a commit that referenced this pull request Sep 3, 2015
@purcell purcell merged commit db80aa5 into melpa:master Sep 3, 2015
@purcell
Copy link
Member

purcell commented Sep 3, 2015

Cool, thanks - merged. :-)

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
awaiting-upstream Awaiting action from an upstream maintainer recipes
Projects
None yet
Development

Successfully merging this pull request may close these issues.

2 participants