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 natural layer #6012

Closed
wants to merge 12 commits into from
Closed

Add natural layer #6012

wants to merge 12 commits into from

Conversation

mkcode
Copy link
Contributor

@mkcode mkcode commented May 8, 2016

Adds the beginnings of a prose layer.

Replaces #5403 according to the discussion in #5996.

@TheBB TheBB mentioned this pull request May 8, 2016
@TheBB TheBB added the New Layer label May 8, 2016
"Workaround for proselint flycheck checker predicate"
prose-proselint-enabled)

(flycheck-define-checker proselint
Copy link
Collaborator

@JAremko JAremko May 8, 2016

Choose a reason for hiding this comment

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

Mb it will better to add this https://github.com/amperser/proselint/blob/master/plugins/flycheck/ to the load path instead of embedding? This way we won't need to maintain the code. But actually it will be even better if we wrap it into a package.

@JAremko
Copy link
Collaborator

JAremko commented May 13, 2016

It looks like flycheck/flycheck#939 will add proselint.

@mkcode
Copy link
Contributor Author

mkcode commented May 13, 2016

It looks like flycheck/flycheck#939 will add proselint.

Nice find @JAremko. The body of the checker, using json, looks a lot better than what I have in this PR. On the other hand, I'm not happy with the modes option to flycheck-define-checker in the PR. This will make it so it is only available in text and markdown modes. If we want it available to be used everywhere, we should use the predicate option instead like I have in this PR.

What is your opinion of what we should do? I guess the ideal would be to use the upstream flycheck checker and replace modes to predicate within spacemacs init?

@JAremko
Copy link
Collaborator

JAremko commented May 13, 2016

This will make it so it is only available in text and markdown modes. If we want it available to be used everywhere, we should use the predicate option instead like I have in this PR.

@mkcode We can use flycheck-add-mode with a config variable. So a user will be able to specify where the linter should be enabled. I'll prototype something. But I think we should wait until the flycheck PR is merged or ask them if there is a better way. Or might be the PR can be improved if you think that it lacks something.

@mkcode
Copy link
Contributor Author

mkcode commented May 13, 2016

@JAremko - Great feedback. 👍 I asked @lunaryorn for instruction for the best way to do this. He should know best.

This seems to be working well. I would like to be able to use proselint
everywhere including for long comments in prog-modes, many of which are
already running multiple flycheck checkers that I would also like. This
function resets proselint's next-checkers in theory on every new buffer.
It allows proselint and the mode defined checkers to all be run.
@mkcode
Copy link
Contributor Author

mkcode commented May 13, 2016

@JAremko - I think I fixed the issue with 25d0a84!!! 🎉 Its working well for me at least. Now it looks like I can leave proselint running, get its feedback in prog modes, and also get my prog mode checkers feedback.

@JAremko
Copy link
Collaborator

JAremko commented May 13, 2016

Great idea. Now we need @syl20bnr feedback.

Interesting why text-mode in :modes doesn't count as the superset mode of all text related modes? Aren't they supposed to be derived from it ?

@mkcode
Copy link
Contributor Author

mkcode commented May 13, 2016

Interesting why text-mode in :modes doesn't count as the superset mode of all text related modes?

@JAremko - flycheck doesn't check for derived modes: https://github.com/flycheck/flycheck/blob/master/flycheck.el#L1601-L1602

@JAremko
Copy link
Collaborator

JAremko commented May 13, 2016

@mkcode I updated readme to the new standard also renamed and moved the layer. New name fits better if we want to extend it in the future.

You can cherrypick it or simply copypasta 😄 mkcode@8dbc974

Also I think you should squash some commits if they aren't particularity interesting.

flycheck doesn't check for derived modes

Too bad. It could'v reduce boilerplate. Thank for pointing out 👍

@mkcode
Copy link
Contributor Author

mkcode commented May 13, 2016

@JAremko - cherry picked your commit. What was the motivation to rename as natural mode? Perhaps I missed something in gitter?

Also - I'll squash later. I need to get to my real job now... :-)

@mkcode mkcode changed the title Add prose layer Add natural layer May 13, 2016
@JAremko
Copy link
Collaborator

JAremko commented May 13, 2016

@mkcode I only renamed the layer. layers on the develop branch are organized in folders. So the layer should go in some category. +lang already has non programming languages so it should fit. And the name of the layer should be broader if we want to add more tools #5996 Better to come up with a perfect name now because renaming a layer that is in use will be a pain in the rear end for sure.

Also - I'll squash later. I need to get to my real job now... :-)

No hurry. We are waiting for the flycheck PR to be merged anyway 😉

@JAremko
Copy link
Collaborator

JAremko commented May 13, 2016

If @syl20bnr is ok with the way it is done and the new name/place we'll also need to rename prose to natural in the code - that I forgot to do

@stephanschubert
Copy link

Any update?

@delaanthonio
Copy link
Contributor

This layer should be called "language" instead of "natural" since this layer is about language, not nature.

@viccuad
Copy link
Contributor

viccuad commented Aug 1, 2017

I would call it prose, as language or natural sounds too open, and "prose mode" is self explicative. I would like my bikeshed blue, too :).

@robbert-vdh
Copy link
Contributor

It would be nice to have other tools for working with natural language (like LanguageTool #8899) integrated into the layer. That would probably require having to put the different tools behind flags as most of them have got external dependencies, but it would make it a lot easier to add more tools without cluttering things up too much.

@stephanschubert
Copy link

stephanschubert commented Dec 25, 2017

What's missing to get this PR merged? Just re-naming sounds doable ;)

;; Reset the proselint next-checkers prop to nil
(setf (flycheck-checker-get 'proselint 'next-checkers) nil)
;; Add the regular mode and predicate checkers for this buffer
(dolist (checker (flycheck-possibly-suitable-checkers))

Choose a reason for hiding this comment

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

flycheck-possibly-suitable-checkers is not available (anymore?) in flycheck 32. I raised a simiilar issue here abingham/flycheck-vale#10 (comment) but it seems that flycheck needs to resolve a long-running issue first.

@duianto
Copy link
Collaborator

duianto commented Sep 17, 2020

The following PRs changes have been applied to the develop branch:
Add a layer for LanguageTool support #8899

@smile13241324
Copy link
Collaborator

I am not sure if integrating proselint is still necessary given that the project looks to be pretty stale, see here https://github.com/amperser/proselint/, last commit is from before 4 years. If it would still be useful, I would volunteer to put it into the spell-checking layer.

Though I would prefer to have the build in flycheck package be extracted to its own package and offered to flycheck in general to take the rest of the emacs world with us. But this is something I can do on the way.

Let me know if this is still needed.

@smile13241324
Copy link
Collaborator

Looks like this is so old and fragmented we will be better of starting anew with a fresh design for a such a layer. Therefore I will close this PR.

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

10 participants