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 option to avoid filtering empty comments #498

Merged
merged 2 commits into from Jun 16, 2017

Conversation

Projects
None yet
2 participants
@davisagli
Contributor

davisagli commented May 19, 2017

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

This comment has been minimized.

Member

pascalduez commented May 20, 2017

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

This comment has been minimized.

Contributor

davisagli commented May 20, 2017

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

This comment has been minimized.

Member

pascalduez commented May 20, 2017

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

This comment has been minimized.

Contributor

davisagli commented Jun 2, 2017

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?

@davisagli davisagli changed the base branch from develop to master Jun 2, 2017

@pascalduez

This comment has been minimized.

Member

pascalduez commented Jun 3, 2017

@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

This comment has been minimized.

Contributor

davisagli commented Jun 15, 2017

I've rebased on master and added a test.

@pascalduez pascalduez merged commit deb6bdc into SassDoc:master Jun 16, 2017

1 check failed

continuous-integration/travis-ci/pr The Travis CI build failed
Details
@pascalduez

This comment has been minimized.

Member

pascalduez commented Jun 16, 2017

Merged.

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

This is delaying the release though.

@davisagli

This comment has been minimized.

Contributor

davisagli commented Jun 16, 2017

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

This comment has been minimized.

Member

pascalduez commented Jun 16, 2017

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

@davisagli

This comment has been minimized.

Contributor

davisagli commented Jun 16, 2017

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

This comment has been minimized.

Member

pascalduez commented Jun 17, 2017

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

@pascalduez

This comment has been minimized.

Member

pascalduez commented Jun 17, 2017

+ sassdoc@2.3.0

@pascalduez pascalduez referenced this pull request Jun 17, 2017

Open

Themes configuration #503

@pascalduez

This comment has been minimized.

Member

pascalduez commented Jun 17, 2017

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

This was referenced Jun 18, 2017

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment