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

Customizable warnings #1978

Closed
guybedford opened this issue Feb 17, 2018 · 12 comments
Closed

Customizable warnings #1978

guybedford opened this issue Feb 17, 2018 · 12 comments

Comments

@guybedford
Copy link
Contributor

I may be jumping the gun here, but perhaps the time is coming to think about warnings customization. Warning flags are pretty much a standard for compilers, so I don't see why Rollup shouldn't have them too.

For example, circular reference warnings may be getting noisy - and it would be nice to just add a --ignore warn-circular or something like that as a CLI flag to disable that one warning in particular without having to implement a custom onwarn hook.

Would be interesting to hear thoughts further, will leave this up to track any complaints of circular reference noise.

@lukastaegert
Copy link
Member

As we already have error codes, maybe we should use those to identify warnings (here: --ignore CIRCULAR_DEPENDENCY)? We would just need to communicate them in some way to the user. Ideally, the CLI could track those warnings and in the end, display something like: To hide these warnings, call rollup with --ignore CIRCULAR_DEPENDENCY --ignore SOME_OTHER_ERROR_CODE. Just vague ideas.

@skiano
Copy link

skiano commented Mar 23, 2018

@guybedford I am experiencing noisy warn-circular warnings, and would love a feature like this. Do you think I should be looking for a way to suppress them, or do you think I should try to eliminate them in the code. (The code seems to function fine, so unless there is some nuanced thing I should be worried about, I would prefer to leave them)

Also If I wanted to help with this feature how might I look into it, considering I am haven't yet spent any time with the source code.

@guybedford
Copy link
Contributor Author

@skiano if you're interested in working on this that would be really amazing. I'd advise we discuss the basic API first to avoid any wasted development time though.

So sketching it out would probably be the first step. Ideally we need this to work through both the API and the CLI, with a pattern for disabling (do we need enabling?) warning codes.

-Wno-warning-name is the C-style precendent here I guess, which we could possibly follow.

possibly warn: { 'warning-name': false } then for the CLI?

Open to completely different suggestions too.

//cc @lukastaegert

@skiano
Copy link

skiano commented Mar 25, 2018

Thanks @guybedford for starting with this sketch. It makes it less daunting to consider contributing.

I am not familiar with the C-style precedent. Is Wno just a familiar prefix for disabling compiler warnings? Would I be in the minority being confused about that prefix?

@guybedford
Copy link
Contributor Author

Perhaps just --no-warningName would work too.

Also the current project convention is to use camelCase even for CLI flags so perhaps --no-warnCircular over --no-warn-circular.

We could even dot warnings too possibly - --no-warn.circular.

@skiano
Copy link

skiano commented Mar 25, 2018

I think following whatever the convention is so far makes sense . So --no-warnCircular seems ok to me. I also think seeing it without the Wno is less confusing 👍

I just cloned everything and started poking around. I imagine it will take some time for me to wrap my head around things. Can I bug you with questions as I go along?

@guybedford
Copy link
Contributor Author

Sure, happy to help.

@skiano
Copy link

skiano commented Mar 28, 2018

@guybedford I started a PR just to keep the discussion going.

@laughinghan
Copy link

Instead of turning off any particular warning entirely, can we please have a way to turn off warnings for code in node_modules/? (Or particular warnings in node_modules/, that would be nice too.)

I actually wouldn't mind at all being warned of circular dependencies in my own code so I can avoid them, but there's no way I'm going to fork a dependency that is otherwise working perfectly just to make a Rollup warning go away.

@derrickb
Copy link

@laughinghan I'd be down for that but we also definitely want to be able to turn off specific warnings... I found this thread because I'm looking to be able to disable "eval" warnings as described here: #2082

@shellscape
Copy link
Contributor

Hey folks. This is a saved-form message, but rest assured we mean every word. The Rollup team is attempting to clean up the Issues backlog in the hopes that the active and still-needed, still-relevant issues bubble up to the surface. With that, we're closing issues that have been open for an eon or two, and have gone stale like pirate hard-tack without activity.

We really appreciate the folks have taken the time to open and comment on this issue. Please don't confuse this closure with us not caring or dismissing your issue, feature request, discussion, or report. The issue will still be here, just in a closed state. If the issue pertains to a bug, please re-test for the bug on the latest version of Rollup and if present, please tag @shellscape and request a re-open, and we'll be happy to oblige.

@rickbsgu
Copy link

rickbsgu commented Dec 30, 2020

An in-file directive would be nice to turn warnings off and turn back on, again. That way, you can disable the warning for specific statements, but leave the rest of the file open to examination:

stmt - ok
/* no-warn=eval */
eval(buffer)    // (normally throws a warning)
/* warn=eval */
stmt - ok

You could put the 'no-warn' directive at the top of the file to disable the warning for the entire file. A global directive would probably have to be an option to the compiler.

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

No branches or pull requests

7 participants