Open pascalberger opened 6 years ago
pascalberger
commented
6 years ago Cake.Markdownlint uses markdownlint-cli which requires node to be installed. Not sure how we can handle this the best. Maybe we can check and run the linting if the tool is available?
gep13
commented
4 years ago I am also going to bump this one from the milestone as well.
gep13
commented
4 years ago @AdmiringWorm you mentioned on the stream last night that it would be good to have this, and it looks like we have an issue for it.
Is this something you would like to see in the timeframe of 2.0.0?
AdmiringWorm
commented
4 years ago @gep13 I would love to have it for 2.0.0, but 1 thing I am not sure about is if whether that support should be added to Cake.Recipe, or Cake.Issues.Recipe. This may be something that maybe should be implemented in the latter.
gep13
commented
4 years ago Oh... looks like you are right. I thought that Cake.Issues.Recipe already had support for Cake.Issues.Markdownlint, but looks like it doesn't. I would think that we would need to get that support added there first, and then do the work of generating the report in Cake.Recipe to then pass through to Cake.Issues.Recipe.
pascalberger
commented
4 years ago There are three things to do here:
If you wan't to have it in 2.0.0, you can already start with point 1 (which IMHO is also the biggest work here). Point 2 is no big deal to implement.
gep13
commented
4 years ago So for the first, we could potentially make use of the following module:
https://www.nuget.org/packages/Cake.Npm.Module/0.1.0-unstable0014
And then use the normal RequireTool method to install the CLI tool when required. Would be good to get a non beta release of that module out though.
AdmiringWorm
commented
4 years ago I'll be honest, and I am a bit hesitant to rely on the Npm.Module here (or rather that cake installs the markdownlint).
We have no guarantee that the user will even have NodeJS installed on their computer (in case of local builds), nor on the CI (although most probably do have it). We first need to determine if the user have NodeJS first, before trying to install anything at all.
If you wan't to have it in 2.0.0, you can already start with point 1
While it would be nice to have it in 2.0.0, I wouldn't consider it any priority. Without support already in reporting, the output from markdownlint would most likely just be output text that most people would ignore.
pascalberger
commented
4 years ago I can add the support in Cake.Issues.Recipe very quick, since issue provider already exists and reporting capabilities come for free. The reason it is not in there yet, is not because of the amount of work, but because of lack of demand yet.
AdmiringWorm
commented
4 years ago @pascalberger The reason it isn't implemented is entirely understandable, and I honestly doubt there would be much demand for it in the future either.
It is more a nice to have feature than anything else, but as I mentioned previously, I wouldn't consider it a priority (neither for Cake.Recipe, nor Cake.Issues.Recipe).
pascalberger
commented
4 years ago Support for markdownlint has been released in Cake.Issues.Recipe 0.3.4
AdmiringWorm
commented
4 years ago Been trying to get this to work, but to have it work correctly and only have 1 output result file (which I think Cake.Issues.Recipe expects) is not possible right now (since ArgumentCustomization is completely ignored on the addin settings).
Currently blocked by https://github.com/cake-contrib/Cake.Markdownlint/issues/69
We can add linting for markdown files using Cake.Markdownlint and output the issues using Cake.Issues.Markdownlint.