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

Disable PugPrinter:formatText vue's specific warnings #172

Closed
bsiguret opened this issue Dec 18, 2020 · 20 comments · Fixed by #384
Closed

Disable PugPrinter:formatText vue's specific warnings #172

bsiguret opened this issue Dec 18, 2020 · 20 comments · Fixed by #384
Assignees
Labels
framework: Vue Related to the framework Vue help wanted We are looking for community help type: enhancement Functionality that enhances existing features

Comments

@bsiguret
Copy link

bsiguret commented Dec 18, 2020

Request / Idea

Make a way to be able to disable these kind of warning:

If you are using Vue, you can ignore this message

Additional Context

Hello,

First of all sorry if it's existing already but I couldn't find a way to do this. We are working on Vue with pug template and Nuxt. Our problem is that Pug warning are spamming our terminal every time our site hot reload with this type of messages:

[PugPrinter:formatText]: Missing expected ). If you are using Vue, you can ignore this message. code:

I thought there would be an option to disable these vue's specifics warning but I couldn't find it

@Shinigami92
Copy link
Member

There is one problem with that: It's more "you may ignore this message" instead of "can"
What if this message was not printed, it it's actually a problem 🤔

We already thought about flags for vue and angular, but until now we tried to avoid these flags

You should add a comment on #109 (it's now in GitHub's new Discussion section)

little sidenote: I'm currently making a bit like holiday from working on GitHub, just responding here and there. Will effectively work again starting January 2021

@Shinigami92 Shinigami92 added framework: Vue Related to the framework Vue type: enhancement Functionality that enhances existing features labels Dec 18, 2020
@bsiguret
Copy link
Author

Yeah, my point is to have a flag for Vue to disable these warning who aren't an actual problem, because vue.

It wouldn't be a problem if they were printed only one time but because of Nuxt hot reload they are literally spamming and polluting our terminal. It may be a specific problem thought. I'll check the Discussion and add a little comment linking to this issue.

Aren't we all ? Thanks for the quick response and enjoy your holidays

@Shinigami92
Copy link
Member

@bsiguret

Thinking about this futher, should I just implement a pugLoggerLevel option?
So users can set it to pugLoggerLevel: 'off' if they really want to?

The default would still be 'info', but this way you could set it to e.g. 'error'.
Downside would be you will ignore ANY warnings.
But I don't really want to provide something like ignore for a framework or ignore error x and y and z.
So I don't have error-codes and I also really don't want them cause I think it would be overkill.

What do you think? What would be the at least okay-ish version for you to be satisfied?

@bsiguret
Copy link
Author

@Shinigami92

Hello, Sorry for the delay. I don't think it is a good solution to off all warnings. Some might be very useful.

@Shinigami92
Copy link
Member

@bsiguret Thank you.
This is helpful feedback and I will take this into account when designing a suitable option.

@Shinigami92 Shinigami92 mentioned this issue Mar 28, 2021
6 tasks
@Shinigami92 Shinigami92 added the help wanted We are looking for community help label Jan 6, 2022
@broofa
Copy link

broofa commented May 23, 2022

[Leaving a breadcrumb here for anyone else that stumbles across this.]

I've worked around this issue by monkey-patching the Logger class thusly:

const { default: PugLogger } = await import('@prettier/plugin-pug/dist/logger');
const {message: originalMessage} = PugLogger.Logger.prototype
PugLogger.Logger.prototype.message = function(level, message, ...optionalParams) {
  // Ignore "debug" log messages
  if (level <= 0) return;

  // Default logging behavior
  originalMessage.call(this, level, message, ...optionalParams);
}

While this is a terrible solution for all sorts of reasons, it does solve the two problems I have, which are:

  1. Remove log spew from the console
  2. Provide a way to programmatically access warnings and errors

'Would love to see a more official API for injecting a custom logger here, but I'm not actually sure what the cleanest way to do that is.

@Shinigami92
Copy link
Member

@broofa Oh wow! You gave me a wonderful idea!
I can export the logger in index and then in a .prettierrc.cjs your could do something like:

// @ts-check
/// <reference path="node_modules/@prettier/plugin-pug/src/prettier.d.ts" />

const pluginPug = require('@prettier/plugin-pug');

pluginPug.logger.setLogLevel(1); // Should be possible to e.g. pass `'ERROR'` or so

/**
 * @type {import('prettier').Options}
 */
module.exports = {
  plugins: [
    require.resolve('@prettier/plugin-pug'),
  ],

  singleQuote: true,
  trailingComma: 'all',

  pugSingleQuote: false,
  // ...
};

I tried it out locally by manually writing into node_modules/.pnpm/@prettier+plugin-pug@2.0.0_prettier@2.6.2/node_modules/@prettier/plugin-pug/dist/index.js 😅
But it seems to work.
I could work on that in next few days, but I would prefer if you or someone else would just open a PR for that so I need only to review it 🙌

@broofa
Copy link

broofa commented May 24, 2022

Any way this could be made configurable via one of the JSON config file options?

JS-based config files suffer from a number of issues:

  • Don't translate well to CLI flags
  • Not easily schema-validated
  • Blurs the line between configuration vs. application
  • "Wait... it has to be CommonJS? But all my code is ESM. How do I even...?"

Edit to add: It would also be nice to be have a way of injecting a custom logger. And have a well-defined structure for log messages. E.g.

{
  message : string,
  level : number,
  filepath ?: string, 
  line ?: number,
  column ?: number,
  excerpt ?: string
}

... something like that. (The end goal here being that callers would have a way to understand and route log messages to other services rather than them always going to stdout.)

@Shinigami92
Copy link
Member

Any way this could be made configurable via one of the JSON config file options?

I highly assume that is not well possible, because the logger may run in a context where you have no access to the options.
Also even if it would be configured by an option from prettierrc it would be configured for every single file again and again.

The logger is currently build in a way that it is for the whole plugin.

With your request to have the possibility to pass a custom logger we would be totally out of config-via-json at all.
When using e.g. pnpm in your project you also need to use e.g. .prettierrc.cjs as you need to configure the plugin via plugins: [require.resolve('@prettier/plugin-pug')].

You have full TS / autocomplete support for cjs files when using // @ts-check + /** @type {import('prettier').Options} */, so you wont loose auto completion.


That all said... I like your idea of having it possible to configure your own logger, your own level and set that via .prettierrc.cjs 🤔
But what I don't like is that every (at least in JS written) prettier plugin would need its own logger.
And now I'm thinking of one @prettier/logger or prettier-logger 👀
Then we could also provide a context filter and say e.g. only log stuff for the @prettier/plugin-pug plugin.

One problem I have is: make a total solution and solve it finally (prettier/prettier#11078) or it is over engineered for a single plugin like mine.

/cc @thorn0

A "programmatic" logger api for prettier plugins may also solve #172 (comment), because we could implement it so that you can consume the warning and filter on it or something like that.


I have so many ideas but I don't want to start it if it is not worth it at the end because this could be very time consuming, theoretically a whole new project to manage and at the end I'm afraid that it could be over engineered.

@broofa
Copy link

broofa commented May 25, 2022

For context, I'm coming at this problem from the perspective of the work we're doing here at CodePen, where we do file processing on behalf of the user. I.e. We run user-supplied content through prettier(+pug) programatically. And, because we want to provide structured feedback to the user about any issues we might find anything a plugin output to the console is either a) log noise that we probably don't care about, or b) issues that we need to capture and surface to the user (e.g. as "squiggly-red-lines" under the offending content).

I'm thinking of one @prettier/logger or prettier-logger 👀

💯 agree here. Definitely seems like the sort of thing that should be common across all plugins.

@Shinigami92
Copy link
Member

a) log noise that we probably don't care about

So do you just want to disable @prettier/plugin-pug logs completely? Or do you still want to see some logs like error?
I could introduce a process.env.PRETTIER_PLUGIN_PUG_LOG_LEVEL or something like this 🤔
With that you could maybe run PRETTIER_PLUGIN_PUG_LOG_LEVEL=ERROR npm run format or whatever
Otherwise could you tell me what specifically you would like to have logged and what not?
Somewhere I mentioned that I could move some stuff to e.g. a trace log, but you shouldn't see debugs anyways until you have process.env.NODE_ENV set to 'test'.

plugin-pug/src/printer.ts

Lines 111 to 112 in d07b9f5

if (process.env.NODE_ENV === 'test') {
logger.setLogLevel(LogLevel.DEBUG);

But actually this was already helpful for me in the past as I could identify some issue problems with that debugging more easily, that is why I debug it

The initial issue description also mentioned warnings like [PugPrinter:formatText]: Missing expected ). If you are using Vue, you can ignore this message. code: but as far as I know I nowadays check for pugFramework option and when it is set to 'vue' is will not warn.

@broofa
Copy link

broofa commented May 26, 2022

So do you just want to disable @prettier/plugin-pug logs completely?

In looking further at this... in our case... yeah, that would probably make sense. See below.

Otherwise could you tell me what specifically you would like to have logged and what not?

There are two categories of errors/warnings we care about:

  1. "Lint" errors - anything that surfaces as a result of parsing the user's content. I.e. that can be associated with a specific line and column in the input source text, and that can be fixed by editing the content in question.
  2. Everything else - this would be configuration errors, missing dependencies, version incompatibilities... that sort of thing.

That latter category are what I would call "fatal errors" - they're errors that prevent the the user content from being linted or formatted - and, as expected, take the form of thrown exceptions. Which is great. That's exactly as expected.

The former category - "lint" errors - are also being thrown as exceptions, but they're... weird. For example, if there's a missing quote (") in the input, prettier.format() throws this String (not Error):

"Error: Pug:2:1\\n 1| div.text( color = primary\\", disabled =\\"true\\" )\\n > 2|\\n-------^\\n\\nThe end of the string reached with no closing bracket ) found.\\n at makeError (/Users/kieffer/codepen/cp/processors/node_modules/pug-lexer/node_modules/pug-error/index.js:34:13)\\n at ...<long stacktrace deleted>..."

The most obvious issue here is that plugin-pug is throwing a stringified version of the Error that pug provides. In doing so, it drops (or at least obfuscates) important information, like the code, line, column, and filename... all of which are definitely of interest to us. For example, here's the error, above, as originally thrown by pug:

Error: Pug:1:52
  > 1| div.text( color =   primary",  disabled  ="true"  )
----------------------------------------------------------^

The end of the string reached with no closing bracket ) found.
    at ... <long stacktrace deleted>... {
  code: 'PUG:NO_END_BRACKET',
  msg: 'The end of the string reached with no closing bracket ) found.',
  line: 1,
  column: 52,
  filename: test.pug,
  src: 'div.text( color =   primary",  disabled  ="true"  )',
  toJSON: [Function (anonymous)]
}

Anyhow... to answer your original question about disabling logging: Yes, given that the errors we care about are actually being thrown as exception, rather than being logged, we could probably just disable logging altogether in our case.

(That said, it's odd that lint errors get thrown as exceptions. Generally speaking, "linting" can involve more than one error, so you typically want to handle things in a way that allows for that, which throwing doesn't. Other tools I've worked with will return a data structure that looks something like {result: '<processed text>', errors: [<array of errors and/or warnings]}.

@Shinigami92
Copy link
Member

I will test out to implement a solution to set logging via env variable, need to test if it's possible to set from outside somehow.
Definitely working solution would still be to set it via .prettierrc.cjs file.

You mentioned lint errors... that confuses me a bit. @prettier/plugin-pug is a prettier plugin and it will just and only format and wont lint anything.
If something should get linted, please search for an eslint plugin.
It is a non-goal to "find" errors in pug files for this plugin.

@Shinigami92 Shinigami92 linked a pull request May 27, 2022 that will close this issue
@Shinigami92 Shinigami92 self-assigned this May 27, 2022
@Shinigami92
Copy link
Member

@broofa I created a { "@prettier/plugin-pug": "2.1.0-rework-logging-feature.1" }
Could you check it out?

For details, see the PR
Please let me know if you can e.g. set PRETTIER_PLUGIN_PUG_LOG_LEVEL='off' or use

// .prettierrc.cjs

import { logger, LogLevel } from '@prettier/plugin-pug'

logger.setLogLevel(LogLevel.OFF)

// ...

@broofa
Copy link

broofa commented May 27, 2022

You mentioned lint errors... that confuses me a bit.

I only mean "lint" in the sense that pug has an AST-parser which spits errors when it encounters ill-formed code. I don't mean linting in the sense that prettier should be alerting callers to such errors.

it will just and only format and wont lint anything

If this is the case, then @prettier/plugin-pug shouldn't throw when pug encounters one of these errors. Instead, it should silently fail to format. (This would be consistent with how other formatters I've encountered behave.)

@Shinigami92
Copy link
Member

@broofa So 2 working days are passed, did you tried it yet? Does it work as expected?

@broofa
Copy link

broofa commented Jun 1, 2022

@Shinigami92 Apologies for the delay. 'Trying to use your new version but it's complaining about LogLevel not being exported:

$ yarn test
yarn run v1.18.0
$ ava

SyntaxError: The requested module '@prettier/plugin-pug' does not provide an export named 'LogLevel'
    at ModuleJob._instantiate (node:internal/modules/esm/module_job:127:21)
    at processTicksAndRejections (node:internal/process/task_queues:96:5)
    at async ModuleJob.run (node:internal/modules/esm/module_job:193:5)
    at async Promise.all (index 0)
    at async ESMLoader.import (node:internal/modules/esm/loader:337:24)
    at null.main (/Users/kieffer/codepen/cp/processors/prettier-2/test/autotest.spec.ts:27:15)
  ✖ No tests found in test/autotest.spec.ts
  ─

error Command failed with exit code 1.
info Visit https://yarnpkg.com/en/docs/cli/run for documentation about this command.

$ yarn list @prettier/plugin-pug
yarn list v1.18.0
warning Filtering by arguments is deprecated. Please use the pattern option instead.
└─ @prettier/plugin-pug@2.1.0-rework-logging-feature.1
✨  Done in 0.79s.

$

[Edit to add: I think this is due to TS enum's not being defined by default... unless you make them const? Relevent SO question.]

@broofa
Copy link

broofa commented Jun 1, 2022

I'm able to set the log level using a string, though - e.g. 'warn', 'off', etc. Other than the above issue, this works great, thank you!

@Shinigami92
Copy link
Member

@broofa I (hopefully) fixed the LogLevel by removing the const and released v2.1.0

@broofa
Copy link

broofa commented Jun 6, 2022

Looks good now. Thanks!

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
framework: Vue Related to the framework Vue help wanted We are looking for community help type: enhancement Functionality that enhances existing features
Projects
None yet
Development

Successfully merging a pull request may close this issue.

3 participants