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

Switch to remark-cli to support .remarkrc and unlock remark version #2008

Closed
wants to merge 1 commit into from
Closed

Switch to remark-cli to support .remarkrc and unlock remark version #2008

wants to merge 1 commit into from

Conversation

kachkaev
Copy link
Contributor

What does this implement/fix? Explain your changes.

I've been trying to add .remarkrc support to fix #965 and was suggested by @wooorm to have a look at remark-cli. As a result, I re-implemented remark beautifier to use remark as an external tool.

The benefit of this approach is not only a local .remarkrc file can be found and loaded, but also that there is no more version lock-in for remark (it used to be v6, which is 2 versions behind the latest).

I don't pretend that this PR is perfect and will be happy to revise it with someone's help.

Does this close any currently open issues?

Closes #965. Related to #2004.

Checklist

Check all those that are applicable and complete.

  • Merged with latest master branch
  • Regenerate documentation with npm run docs
  • Add change details to CHANGELOG.md under "Next" section
  • Added examples for testing to examples/ directory
  • Travis CI passes (Mac support)
  • AppVeyor passes (Windows support)

@kachkaev
Copy link
Contributor Author

@wooorm there's one thing that's working not as I've been expecting and I'm not sure if this is a peculiarity of remark or if I'm just missing anything in remark-cli args.

When I define a rule (e.g. [require('remark-lint-fenced-code-marker'), '~'] inside my .remarkrc.js, I start seeing warnings related to this rule in atom's interface and the command line, which is great. In this case I also expect remark to fix the rule if possible, just as this happens for eslint --fix. However, although the rule is detected by remark-cli and the warning is shown, certain fixable characters remain unchanged and the warnings persists. I've seen that remark has got a stringify module that takes part in forming the output, but I'm not sure how to make it simply follow all the existing linting rules without any extra configuration.

What would you suggest?

"use strict"
Beautifier = require('./beautifier')

module.exports = class Remark extends Beautifier
name: "Remark"
link: "https://github.com/wooorm/remark"
link: "https://github.com/remarkjs/remark"
Copy link
Contributor Author

Choose a reason for hiding this comment

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

The project has been moved to an org in ≈ Dec 2017

serializedSettings = JSON.stringify(settings).slice(1, -1)
remarkArgs.push('--no-config', '--setting', serializedSettings)

return new Promise((resolve, reject) =>
Copy link
Contributor Author

Choose a reason for hiding this comment

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

A new promise object is not really necessary here at this point, but it is helpful to integrate the new implementation of the beautifier with #1990

@wooorm
Copy link

wooorm commented Jan 13, 2018

When I define a rule (e.g. [require('remark-lint-fenced-code-marker'), '~'] inside my .remarkrc.js, I start seeing warnings related to this rule in atom's interface and the command line, which is great. In this case I also expect remark to fix the rule if possible, just as this happens for eslint --fix. However, although the rule is detected by remark-cli and the warning is shown, certain fixable characters remain unchanged and the warnings persists. I've seen that remark has got a stringify module that takes part in forming the output, but I'm not sure how to make it simply follow all the existing linting rules without any extra configuration.

Yes, remark-lint just warns, it doesn’t change anything. remark-stringify (used in remark-cli, by the remark library itself, and here as well) does support some settings to beautify a certain way.
Some rules are aligned with remark-stringify settings. Others are not, and as of yet require a human to change something.

There’s also no code yet to map lint rule settings to remark-stringify settings, but that would be useful for other tools as well!

Alternatively, prettier can now format markdown as well, and it’s based on remark. It has a very nice line wrapping algorithm, that remark itself doesn’t have yet!

Does that help? Let me know if I can be of more help!

@kachkaev
Copy link
Contributor Author

kachkaev commented Jan 13, 2018

Thanks for the explanation @wooorm. Hmmm I did not know that prettier can work with markdown via remark, interesting.

To be honest, my overall goal is to come up with an environment that’s similar to what prettier-eslint provides in atom. There I’ve got some rules defined in .eslint, which influence the result of auto-formatting when I save a js file. What would you suggest to achieve something similar for markdown with .remarkrc?

@kachkaev
Copy link
Contributor Author

ping @wooorm :–)

@wooorm
Copy link

wooorm commented Jan 26, 2018

Sorry for the slow response! You need some custom function, I guess? I’m not sure, it would need some work figuring out what remark-stringify settings correspond to remark-lint rules. How can I help?

@stevenzeck
Copy link
Contributor

@wooorm sorry I was tagging you in the wrong thread, it should have been here. Can remark (based on unified) format markdown and support a .remarkrc file for configuration? Or is the latter only available in remark-cli?

@wooorm
Copy link

wooorm commented Jan 26, 2018

.remarkrc files are only supported by remark-cli. See remark-cli for more details!

@kachkaev
Copy link
Contributor Author

kachkaev commented Jan 30, 2018

@wooorm so can we somehow use .remarkrc to guide beautification with remark-cli? Just wondering what to do with this PR. It was supposed to switch users to an external remark-cli and thus give them control over the result of formatting with .remarkrc. However, now I'm not sure about the usefulness of what I did 😆

@wooorm
Copy link

wooorm commented Jan 30, 2018

Well, the settings could be supported, and if they were, would allow humans to define how atom-beatify formats markdown through their .remarkrc

You could also allow the plugins. That could allow weird things though. Say someone has a pipeline to transform markdown to HTML, if atom-beautify respected that config, all files would end up as HTML!

But first: why do you want .remarkrc files?

@kachkaev
Copy link
Contributor Author

kachkaev commented Jan 30, 2018

I'd like to enable the same functionality as prettier-eslint provides. That is to allow a team to deviate from the 'industry standard' linting rules but still be able to autoformat the source files. Someone's editing policy may require ** for bold text but for someone else a better choice is __ for their own reasons.

When I contribute to a third-party project documentation, I'd like to be able to make their linter happy in one key press rather than by fiddling with micro-editing of symbols and spaces (I may have a habit of padding lists with two spaces, but that's not what the community I contribute to expects). Such a situation is when atom-beautify together with .remarkrc would be a perfect tool. Because .remarkrc exists in some repos for running npm run lint:md in CI already, it feels natural to employ the same file during beautification as well (just like .eslintrc is used both by npm run lint:js and prettier-eslint inside Atom).

Of course, in an ideal world we would all be just using some unified formatting rules, but unfortunately we're quite far from that. Prettier has been trying to standardise JS files for some time but that still does not work well and ends up with lots holy wars in their issues on github 😄

@wooorm
Copy link

wooorm commented Jan 30, 2018

Well, in that case .remarkrc is the solution right?

Now, the settings just supply info the the parser (e.g., footnotes support) and compiler (aka stringifier, like which marker to use for lists, *, +, or -).

plugins are also found in .remarkrc files, such as remark-toc, to autogenerate a table of contents if a Table of Contents header is found.

Other plugins, like the remark-lint rules, just warn about things.

People are free to set up remark lint rules exactly like the compilation settings (or not, but that’s weird). However, I don’t think that should be a concern to atom-beautify.

Anyway: the best way to do this is by depending on unified-engine I think!

@kachkaev
Copy link
Contributor Author

kachkaev commented Jan 30, 2018

I agree that .remarkrc is a solution. I'm just struggling to figure out how to make remark-cli read the plugins and rules from .remarkrc and then --output the result, but without applying toc or other breaking changes like converting everything to HTML. It feels like this feature may naturally be a part of Remark ecosystem, and so atom-beautify will just call remark-cli with specific arguments. Actually, that's what already happening in this PR :–)

@wooorm
Copy link

wooorm commented Jan 30, 2018

What’s wrong with remark-toc, if people have that set up? “breaking changes like converting everything to HTML”, who is to decide what constitutes a breaking change, and what an “okay” change like fixing some line-wrapping for example?
I feel that if people have “breaking things” in their .remarkrc, they explicitly did so 🤷‍♂️

@kachkaev
Copy link
Contributor Author

kachkaev commented Jan 30, 2018

Could it be that people add remark-toc etc. to .remarkrc because they need to export the entire project somewhere, but then want to simply fix their linting warnings with another command? These tasks seem to have different expected outcomes, but share a config file, at least partially. Fixing linting errors in markdown is similar to running eslint --fix or prettier-eslint (that is get me the list of errors and warnings in the source and then do whatever possible to reduce them and update the files).

@stevenzeck
Copy link
Contributor

@kachkaev I checked out your branch locally and, apart from cwd being invalid, it looks to read from a .remarkrc file as you intended. What specifically are you looking to do at this point (or give an example of what your setup is and what it's doing that you don't)?

@wooorm
Copy link

wooorm commented Jan 30, 2018

You could make a .remarkrc config file do anything, but remark-cli and .remarkrc files only really make sense if you’re working with markdown input and output.

I have both formatting stuff like remark-toc (eslint --fix) and linting stuff like remark lint (eslint’s unfixable rules). In my opinion, remark-toc updates my table of contents according to rules I have set up. I feel that that plugin is just as much like eslint --fix and prettier, in that it fixes my table of contents!

@kachkaev
Copy link
Contributor Author

I feel that that plugin is just as much like eslint --fix and prettier, in that it fixes my table of contents!

@wooorm yeah the toc plugin is perhaps something similar to eslint --fix indeed. I did not think that TOC can re-generate itself multiple times without breaking things when a TOC has already been generated, but if that's how it works, great. However, we can't say the same about html-generating plugins, unfortunately. In any case, turning .remarkrc statements like [require('remark-lint-heading-style'), 'setext'] or [require('remark-lint-ordered-list-marker-value'), 'one'] into some kind of eslint --fix still feels like a missing feature.

@szeck87 .remarkrc is being read indeed, it's just not working as I originally expected because linting rules do not affect the beautification really 🤔

@kachkaev
Copy link
Contributor Author

kachkaev commented Jan 30, 2018

A side-thought: is any preference given to a locally installed remark-cli? Or does @exe("remark") grab a globally installed instance in all cases?

@stevenzeck
Copy link
Contributor

@kachkaev It won't read any locally installed version at all. I believe it is possible to do so, but it requires a lot of new code since it checks the version of the executable before it even hits the beautify function.

@kachkaev
Copy link
Contributor Author

Right. Do other beautifiers give preference to locally installed executables? Or is this something against atom-beautify philosophy?

@stevenzeck
Copy link
Contributor

The closest thing I can find is cljfmt and it's completely out of date (using run instead of exe). It doesn't use a global, just a local.

@kachkaev
Copy link
Contributor Author

Maybe we could use a global-only instance of remark for now then. However, may be a good practice to search for an executable in project deps first and give that instance a preference. Have seen in a couple of places; prettier-atom is on top of my head.

@stevenzeck
Copy link
Contributor

Where in prettier-atom specifically?

@kachkaev
Copy link
Contributor Author

I did not inspect their source, yet, but this conclusion comes from my experience and from these options in preferences:

@stevenzeck
Copy link
Contributor

That just looks like it won't format if you don't have prettier as a dependency in the directory of where you're working.

Anyhow, like I mentioned earlier, ideally we don't use executables unless there is no other alternative. If you can find a way to get the cli version to run within node, that would be great. Otherwise I'll defer . to @Glavin001 on what to do.

@wooorm
Copy link

wooorm commented Feb 3, 2018

@kachkaev

In any case, turning .remarkrc statements like [require('remark-lint-heading-style'), 'setext'] or [require('remark-lint-ordered-list-marker-value'), 'one'] into some kind of eslint --fix still feels like a missing feature.

Agreed! Related to remarkjs/remark-lint#82!

@Glavin001
Copy link
Owner

Anyhow, like I mentioned earlier, ideally we don't use executables unless there is no other alternative. If you can find a way to get the cli version to run within node, that would be great.

I would agree with @szeck87 .

.remarkrc files are only supported by remark-cli. See remark-cli for more details!

However, in this case using remark-cli may be required to support .remarkrc.

This route is acceptable as long as users still have the option to configure via .jsbeautifyrc or Atom-Beautify package settings view.

@@ -185,7 +185,6 @@
"open": "0.0.5",
"prettydiff2": "^2.2.7",
"pug-beautify": "^0.1.1",
"remark": "6.0.1",
Copy link
Owner

Choose a reason for hiding this comment

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

@kachkaev @szeck87 : I think we can support both Remark-CLI and be backwards compatible with remark as a Node.js dependency.

Since the Remark CLI is now an Executable we should be able to determine if it is installed or not.
See

@isInstalled = true
@version = version
)
.then((version) =>
@info("#{@cmd} version: #{version}")
version
)
.catch((error) =>
@isInstalled = false

I'm thinking something along the lines of:

isCliInstalled = @exe("remark").isInstalled

if isCliInstalled
  # ...
  @exe("remark").run(remarkArgs...
  # ...
else
  # ...
  cleanMarkdown = remark().process(text, options).toString()	
  # ...

I would prefer to add new features while being backwards compatible. Removable of remark as a builtin dependency is considered a breaking change to me. However, there is value in using Remark CLI directly 👍, such as the configuration file.

@kachkaev
Copy link
Contributor Author

kachkaev commented Mar 2, 2018

@Glavin001 will you have time to finish these changes by a chance? I'm afraid I'll be quite busy for the next several weeks. Feel free to take over this PR and merge it! And thanks for your review!

@stale
Copy link

stale bot commented May 1, 2018

This issue has been automatically marked as stale because it has not had recent activity. If this is still an issue, please add a comment. It will be closed if no further activity occurs. Thank you for your contributions.

@stale stale bot added the stale label May 1, 2018
@Glavin001 Glavin001 removed the stale label May 4, 2018
@stevenzeck
Copy link
Contributor

@kachkaev do you still want to move forward with this, or just use Prettier combined with your other PR for formatting code blocks in markdown?

@kachkaev
Copy link
Contributor Author

Hi @szeck87! I have switched to Prettier and am currently betting on its use with plugins. I won't be able to finish this PR but you are free to continue with its implementation.

@stevenzeck
Copy link
Contributor

OK no problem, I'll close this out.

@stevenzeck stevenzeck closed this May 23, 2018
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.

.remarkrc file is not respected
4 participants