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

feat: add --check flag, deprecate --no-check flag #11343

Closed
wants to merge 1 commit into from

Conversation

kitsonk
Copy link
Contributor

@kitsonk kitsonk commented Jul 9, 2021

Ref #11340

@kitsonk kitsonk requested a review from bartlomieju July 9, 2021 06:55
Copy link
Member

@lucacasonato lucacasonato left a comment

Choose a reason for hiding this comment

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

I am not in favor of landing this as is in a patch release. It silently changes the behaviour of many CI pipelines.

Copy link
Member

@lucacasonato lucacasonato left a comment

Choose a reason for hiding this comment

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

I think all unit and integration tests should have check enabled.

Copy link
Contributor

@caspervonb caspervonb left a comment

Choose a reason for hiding this comment

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

As the plan is now per the referenced issue, this will silently disable type checking in our std smoke tests and cli unit tests when we make it default.

So we patch that here sure but the same applies to every other CI workflow out there so we'll end up having CI matrices that go up to 1.12 and then after 1.12 it is a different CLI interface?

This will also completely break deno test --no-run and turn it into a no-op.

@kitsonk
Copy link
Contributor Author

kitsonk commented Jul 9, 2021

To be clear this only adds --check and deprecates --no-check, as stated in the issue, there will be two PRs. The second will change the default behaviour without flags.

Also, it is planned for a minor release, not a patch release.

@kitsonk
Copy link
Contributor Author

kitsonk commented Jul 9, 2021

As the plan is now, this will disable type checking in our std smoke tests and cli unit tests when we make it default.

Not if the yet to be raised PR adds --check.

Also this will completely break deno test --no-run and turn it into a no-op.

Not necessarily. You are assuming something that doesn't exist yet. It is a valid concern that can be addressed in the PR the doesn't exist yet.

Again, as I stated in the issue, the plan was to this PR and then figure out how disruptive the changes would be for the follow up PR before committing to when it would occur.

Copy link
Collaborator

@nayeemrmn nayeemrmn left a comment

Choose a reason for hiding this comment

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

I think this change should be coupled with the other. It adds a flag that does nothing and deprecates a flag that isn't replaceable yet. Seems like an unreliable way of gauging disruption, if that's the purpose.

@ry
Copy link
Member

ry commented Jul 9, 2021

We have decided not to land this for the 1.12 release. There's clearly some controversy here, so we'll lay out the plan more carefully in an issue, discussing the reasoning behind the no-check-by-default change and a plan for how we can responsibly introduce this change.

Tentatively adding this PR to the 1.13 milestone.

Let's move further discussion back to #11340.

@ry ry added this to the 1.13.0 milestone Jul 9, 2021
@bartlomieju bartlomieju removed this from the 1.13.0 milestone Aug 6, 2021
@kitsonk
Copy link
Contributor Author

kitsonk commented Aug 23, 2021

Closing in favour of #11810

@kitsonk kitsonk closed this Aug 23, 2021
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

Successfully merging this pull request may close these issues.

7 participants