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

Unbreak the build #20358

Closed
wants to merge 1 commit into from
Closed

Unbreak the build #20358

wants to merge 1 commit into from

Conversation

odersky
Copy link
Contributor

@odersky odersky commented May 7, 2024

For some reason, BestEffortTastyWriter does not compile since PlainFile does not have a delete method, only File does.

For some reason, BestEffortTastyWriter does not compile since PlainFile does not have a delete
method, only File does.
@odersky odersky requested a review from jchyb May 7, 2024 19:40
@odersky
Copy link
Contributor Author

odersky commented May 7, 2024

I wonder: Why did the original failing code get through the CI?

@odersky
Copy link
Contributor Author

odersky commented May 7, 2024

I see now it's already handled in #20355. On the other hand, maybe the fix here is preferable. I see that lots of classes have unimplemented delete methods. So maybe it's not such a good idea to add it, after all?

@odersky
Copy link
Contributor Author

odersky commented May 7, 2024

In any case, we should merge either this PR or #20355. I leave it to you to decide which.

@jchyb
Copy link
Contributor

jchyb commented May 7, 2024

It was an old PR, which had a passing CI run on top of old main without the changes. I didn't realize that when I was asked to review it earlier today. I prefer this fix, but I wanted to have the main fixed as soon as possible so I tried to revert after noticing. Seems like both CIs got stuck on test_windows, but I believe those errors to be spurious. I will try to restart the failed tasks, and I will merge the PR that will have a successful run (but preferably this one).

@odersky
Copy link
Contributor Author

odersky commented May 7, 2024

It was an old PR, which had a passing CI run on top of old main without the changes.

I see, that explains it. Maybe we should have a recommendation that old PRs are re-tested before merge. On the other hand, that would have added to the crazy build load we had today.

@odersky odersky closed this May 8, 2024
jchyb added a commit that referenced this pull request May 17, 2024
Previously reverted due to conflicts on main just before 3.5.0 cutoff. 
Original PR by @OlegYch (#20193). I
added an additional commit fixing the issues (like in #20358)
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.

None yet

2 participants