SassDoc / sassdoc

Release the docs!
http://sassdoc.com
MIT License
1.41k stars 56 forks source link

Add option to avoid filtering empty comments #498

Closed davisagli closed 7 years ago

davisagli commented 7 years ago

This adds an option so that a theme can request to see all comments including the ones that are normally filtered out due to not being attached to a Sass block (see #477)

pascalduez commented 7 years ago

Hey,

thanks for the PR.

Just a really quick though: I don't think that should come from the theme, bottom to up config for parser related things does not feel right. I would rather put this in the main config, either with the --config arg or a .sassdocrc.

Also such PR would need unit tests :)

Ping @SassDoc/owners for other opinions.

davisagli commented 7 years ago

I put it as an option on the theme because we would like people to be able to use our theme without needing to worry about adding another option to their sassdoc configuration. But we are considering turning the theme into a wrapper around the Sassdoc API rather than just a theme, so I'm okay with making that change.

I would like to add a unit test, but was not sure how to approach it since the code that needs to be tested is buried inside the parser's stream function. Do you have a suggestion of how to do it? Or a pointer to an existing test of that function?

pascalduez commented 7 years ago

I put it as an option on the theme because we would like people to be able to use our theme without needing to worry about adding another option to their sassdoc configuration.

Okay, I understand. I'm trying to wrap my head back at what we've done actually :) As a matter of fact there's already config keys passed bottom up, like custom annotations or the privatePrefix one.

davisagli commented 7 years ago

I'm going to look at this again to add a test, but I am confused about which branch I should target with the pull request. DEVELOPMENT.md says to make a pull request targeting the develop branch, but it looks like the develop branch is somewhat out of date and recent releases have been made from the master branch. Which is right?

pascalduez commented 7 years ago

@davisagli Yeah sorry about the confusion, forget about the develop branch, it's outdated. But we had to keep it cause there's a feature in there we will need to cherry-pick somehow...

davisagli commented 7 years ago

I've rebased on master and added a test.

pascalduez commented 7 years ago

Merged.

I fixed the linting, but the unit test is not passing. Will look into it later.

This is delaying the release though.

davisagli commented 7 years ago

Oops, thanks for fixing the lint errors. The build appears to have failed due to a network problem reaching the npm mirror, so hopefully it will pass if you re-run the build.

pascalduez commented 7 years ago

@davisagli The unit test does not pass locally. The command to run the tests: make test.

davisagli commented 7 years ago

make test is successful for me locally. I even deleted node_modules and re-ran yarn to make sure I have the right dependencies. How does it fail for you?

pascalduez commented 7 years ago

And now it's passing today... another computer.

pascalduez commented 7 years ago

+ sassdoc@2.3.0

pascalduez commented 7 years ago

Well here's the explanation, it's failing on node 4 https://travis-ci.org/SassDoc/sassdoc/jobs/243928836