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

Only log warnings #244

Merged
merged 1 commit into from Dec 19, 2018
Merged

Only log warnings #244

merged 1 commit into from Dec 19, 2018

Conversation

stefanfisk
Copy link
Contributor

Currently messages are only logged if there are warnings, but if there are then all messages are logged.

@stefanfisk
Copy link
Contributor Author

stefanfisk commented Oct 21, 2018

The failing test is caused by #123.

@RyanZim
Copy link
Collaborator

RyanZim commented Oct 22, 2018

From https://github.com/postcss/postcss-reporter#purpose:

By default, only warnings are logged. If you would like to see more messages, you can change the filter function.

Isn't this actually working this way?

@stefanfisk
Copy link
Contributor Author

Not for me at least. postcss-reporter is, as far as I can tell, only used for formatting the messages:

const reporter = require('postcss-reporter/lib/formatter')()

The reason I submitted the PR was that I am seeing all of the dependency messages printed as undefined [undefined] if there was also a warning message when using postcss-import. Merely importing an empty file is enough to recreate the issue.

I was kinda hyper focused yesterday, so I realise now that opening an issue and actually describing the situation would have been clearer, sorry about that :/

@RyanZim
Copy link
Collaborator

RyanZim commented Nov 13, 2018

Tests are failing because you didn't run npm run format to prettify your code, not because of #123

Currently messages are only logged if there are warnings, but if there are then all messages are logged.
@stefanfisk
Copy link
Contributor Author

Sorry for the super late reply to this, and for my overall sloppiness with the PR 😕

When I run npm run format it does not fix index.js, and the only thing that fails for npm run ci is "error › invalid --config".

However, if I change the prettier script to prettier --single-quote --no-semi *.js **/*.{js,md} I get the same output that Travis is generating above. After investigating it seems like maybe the glob should be quoted? https://prettier.io/docs/en/cli.html

I have updated the PR with a properly formatted patch, so now everything should be OK, but you might want to be aware of the prettier issue I encountered.

@RyanZim
Copy link
Collaborator

RyanZim commented Dec 17, 2018

@stefanfisk What's your OS?

@coveralls
Copy link

Coverage Status

Coverage increased (+0.5%) to 75.524% when pulling f289373 on stefanfisk:patch-1 into 54cdf3a on postcss:master.

@stefanfisk
Copy link
Contributor Author

@RyanZim macOS Mojave. I am running zsh, but as far as I can remember the issue was reproducible when I switched to Bash. Quoting the glob also worked if I remember correctly, but I have not investigated how exactly npm executes the scripts and what the implications of not quoting are.

@RyanZim
Copy link
Collaborator

RyanZim commented Dec 17, 2018

Ah, makes sense. Globstar (**) support isn't turned on by default in most shells. If we quote it, prettier expands the glob itself, if we don't, the shell tries to expand it, but messes up.

Will fix.

@RyanZim RyanZim merged commit 9ab9a2f into postcss:master Dec 19, 2018
@stefanfisk stefanfisk deleted the patch-1 branch December 19, 2018 21:38
@nodaguti
Copy link

Coinsidentally I tried to fix the same issue from the postcss-reporter side, and am happy with it being resolved anyway 😄
postcss/postcss-reporter#46

Thanks to both of you!

@XhmikosR
Copy link
Contributor

XhmikosR commented Jan 4, 2019

@RyanZim this causes empty lines printed. https://travis-ci.org/twbs/bootstrap/jobs/475278409#L666-L695

@stefanfisk
Copy link
Contributor Author

@XhmikosR what have done to verify that this change is the cause of the blank lines?

@XhmikosR
Copy link
Contributor

XhmikosR commented Jan 4, 2019

Obviously I tested it.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

None yet

5 participants