Skip to content

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
ContributorAuthor

stefanfisk commented Oct 21, 2018

The failing test is caused by #123.

@RyanZim
Copy link
Collaborator

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
ContributorAuthor

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

constreporter=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

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
ContributorAuthor

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

@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
ContributorAuthor

@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

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.

@RyanZimRyanZim merged commit 9ab9a2f into postcss:masterDec 19, 2018
@stefanfiskstefanfisk 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

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

@XhmikosRXhmikosR mentioned this pull request Jan 4, 2019
@stefanfisk
Copy link
ContributorAuthor

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

@XhmikosR
Copy link
Contributor

Obviously I tested it.

Sign up for freeto 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.

5 participants

@stefanfisk@RyanZim@coveralls@nodaguti@XhmikosR