icerpc / slicec

The Slice compiler library
Apache License 2.0
13 stars 5 forks source link

Use lint terminology #613

Closed bernardnormier closed 1 year ago

bernardnormier commented 1 year ago

The Slice compiler should adopt a lint terminology similar to the Rust compiler's.

lint = suspicious expression

Allow = set the lint level for a lint (lint category?) to Allow Warn = set the lint level for a lint (lint category) to Warn

And for all our current lints (lint categories?), the default lint level is Warn.

Not sure if we should distinguish between lint and lint categories.

pepone commented 1 year ago

Warn = set the lint level for a lint (lint category) to Warn

Our Warn option does something different, it means to treat warnings as errors. If we adopt rust lint terminology I think the proper option is -D, --deny LINT Set lint denied. If course we cannot use -D because it is already in use for defining symbols.

InsertCreativityHere commented 1 year ago

Depends on the context for me. If we're in a technical setting like the Language Reference, I'd call them "lint categories". But for plain-speech, "lint" is sufficient.

InsertCreativityHere commented 1 year ago

If we adopt rust lint terminology I think the proper option is

Yes, maybe we should switch --warn-as-error and --define to use a lowercase letter for their 'short' options.

Then use uppercase for all the lints configuration options: -A = allow, -W = warn, D = deny

Or vice versa, as long as all the lint flags are in the same casing is all that matters to me.

bernardnormier commented 1 year ago

For me, -D is expected to set a preprocessor symbol. Having -D mean "set lint level to Deny" would be odd.

pepone commented 1 year ago

Yes, maybe we should switch --warn-as-error

I think warn-as-error isn't consistent with the lint terminology. In the lint terminology it should be -D, --deny

having -D mean "set lint level to Deny" would be odd.

Agree, but what will you use to set the warning level to error using the lint terminology?

InsertCreativityHere commented 1 year ago

For me, -D is expected to set a preprocessor symbol. Having -D mean "set lint level to Deny" would be odd.

Then we use lowercase for the lint levels then, and keep the other options as-is. Also fine with me. -a = allow, -w = warn, -d = deny

-D = define -W = treat warnings as errors

InsertCreativityHere commented 1 year ago

I think warn-as-error isn't consistent with the lint terminology

Are you saying we should get rid of warn-as-error? I guess it is just a poor-man's deny until we add it, so maybe indeed we should not even offer it.

pepone commented 1 year ago

Are you saying we should get rid of warn-as-error?

Yes, If we add -d = deny I don't see why we will want to keep W/warn-as-error. I think they are mostly equivalent but use conflicting terminology.

InsertCreativityHere commented 1 year ago

Yes, If we add -d = deny I don't see why we will want to keep W/warn-as-error

Sure. Being able to promote warnings to errors doesn't seem necessary for a 0.1. And it's easier to just not provide this option than remove/replace it in the future.

InsertCreativityHere commented 1 year ago

This was implemented in #620 with some slight differences in terminology:

A "lint" is a tool running a check, or the rules it's checking your code against. NOT the suspicious expressions it detects. You run lints to detect suspicious expressions. Instead of running a tool to detect lints.

InsertCreativityHere commented 1 year ago

I opened an issue (#622) where we can continue any discussions about how to name the warn/deny attributes we'll want to add in the future.