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

Remove compiler warnings #7652

Closed

Conversation

bcardiff
Copy link
Member

@bcardiff bcardiff commented Apr 8, 2019

There was a core team discussing where the consensus was to

  • Keep @[Deprecated]
  • Remove compiler checks on deprecated methods
  • Add, at least initially, the deprecation checks as a separate compiler tool

This PR reverts #7626 and #7596, but keeps the compiler fixes found along the way.
It closes #7650 since that is embedded in the current implementation.

The deprecation check tool will probably come after 0.28

@j8r
Copy link
Contributor

j8r commented Apr 8, 2019

Why, what was the conclusion? Can't it be available behind a feature flag?

src/annotations.cr Outdated Show resolved Hide resolved
@straight-shoota
Copy link
Member

@j8r While discussing the existing implementation we decided that we want to make it better and work on a different solution as a compiler tool. But we don't want to delay 0.28.0 to wait for this replacement.
Changing the existing implementation to an opt-in preview feature was considered, but since that code won't stick around anyway it makes more sense to remove it right away as it can't serve as a preview for a different tool anyway.

@j8r
Copy link
Contributor

j8r commented Apr 8, 2019

I guess having proper RFCs would help communication, and understand the design decisions... The community won't know what happens in a black box channel.

@bcardiff bcardiff force-pushed the feature/defer-compiler-warnings branch from eb325dc to 223d5f5 Compare April 8, 2019 23:50
@bcardiff
Copy link
Member Author

bcardiff commented Apr 9, 2019

@j8r I've just created #7655 for that. The original PR served as discussion, it had substantial changes from the opening to the final approval and merge, but there were some more opinions to come. At the end of the day, the story is still moving forward.

@straight-shoota
Copy link
Member

I think we should also drop the strict validation of Deprecated annotation arguments from the compiler. Let's allow having yet-unspecified named arguments. This enables us to discuss adding additional arguments and already use them in code without breaking it for a 0.28.0 compiler.

We don't really gain anything from the strict validation, especially since it's only implemented on methods anyway.

@bcardiff
Copy link
Member Author

bcardiff commented Apr 9, 2019

@straight-shoota if there are no restrictions, then is hard to give semantics.

Copy link
Member

@straight-shoota straight-shoota left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

This is okay to merge in any case 👍

@Sija
Copy link
Contributor

Sija commented Apr 10, 2019

@straight-shoota I'd wait for resolution of the issue #7655, since this approach raised a lot of valid questions, which IMO should be tackled before any merge happens.

@straight-shoota
Copy link
Member

@Sija The decision was not to delay 0.28 for #7655. There is no hurry for this feature. Let's take some time and make it a great one. The discussion can continue without any rush for meeting the milestone.

@asterite
Copy link
Member

@straight-shoota What if we let the warnings be integrated in the compiler, but opt-in for 0.28.0? Then people can try them out and if they turn out to be very useful we can make them opt-out for 0.29.0. Or even the other way around: opt-out for 0.28.0, and if they turn out to be very annoying we can make them opt-in for 0.29.0 or even 0.28.1.

I'd really like to try it opt-out first. If we don't try and experiment with new things we'll never know what's the real reaction or usefulness of this. And, as I said somewhere else, you can always pass a flag to silence the warnings.

@straight-shoota
Copy link
Member

I was personally okay with having it as opt-in. But the consensus was to drop this compiler feature for 0.28.0 entirely. I'd strongly recommend we stick to this decision instead of burning our fuel on discussions about whether we should diverge. Let's focus our attention on releasing 0.28.0 and designing the deprecation warnings feature in #7655.

Shipping this as an opt-out feature as currently in master is a no-go from @RX14 and me. Reports for every invocation has the potential of spamming the console output with pages of deprecation messages. We should be really sure about how it behaves on a larger scale before shipping such an annoying release.

@asterite
Copy link
Member

asterite commented Apr 10, 2019

@straight-shoota No, that was what you and @RX14 wanted, but me, @bcardiff and @ysbaddaden want it as opt-out. That's not consensus.

@j8r
Copy link
Contributor

j8r commented Apr 10, 2019

I think including it as opt-in is a good compromise between complete removal and enabling it by default with opt-out.
It's close to the feature flag, but with more exposure, being documented in the CLI.

@Sija
Copy link
Contributor

Sija commented Apr 10, 2019

I have no insight into your decision process guys, but I definitely agree with @asterite on that, only if everyone are on board with the decisions at hand you can call it a consensus, not before.

@straight-shoota
Copy link
Member

Well, not not everyone was present in the discussion. It was an adhoc meeting and it became apparent that we needed a decision on what to do about the upcoming release. After an intensive discussion of 2 hours or so we agreed on the course of action. Which was then presented in this PR and the dicsussion about the content continued in #7655. And I think the result should be okay for everyone - and so is probably #7661. Neither is preferred by everyone, but both should be acceptable.

@bcardiff bcardiff removed this from the 0.28.0 milestone Apr 10, 2019
@bcardiff bcardiff deleted the feature/defer-compiler-warnings branch April 11, 2019 20:37
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Projects
None yet
Development

Successfully merging this pull request may close these issues.

5 participants