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

[Process] What we learned in C# 7 #960

Closed
BillWagner opened this issue Sep 29, 2023 · 5 comments
Closed

[Process] What we learned in C# 7 #960

BillWagner opened this issue Sep 29, 2023 · 5 comments
Assignees
Labels
meeting: discuss This issue should be discussed at the next TC49-TG2 meeting

Comments

@BillWagner
Copy link
Member

Updated process for ECMA standardization for C# 8

This issue captures observations from moving to GitHub / Markdown / public for the C# 7.3 standard. It identifies some suggestions where we can improve.

Overall, the process was an improvement. It was easier to have multiple tasks running in parallel, assigned to different members of the committee. We built a number of tools to help automate some of our tasks. We also had more early feedback from others regarding our work.

Friction points

There are three main friction points that slowed our work:

  1. Creating initial PRs for all the C# 7 features. Rex has already done a great deal of advance work to create draft PRs for all (or almost all) C# 8.0 features.
  2. Ensuring example code runs as noted. Mostly addressed by the new testing tools.
  3. Getting from "ready to review" to "ready to merge".

The third slowed us down the most. There were a few underlying causes:

  • Some PRs were very complex. The were harder to create and to review for completeness.
  • Some features had significant interaction, so PRs couldn't be complete without other features.
  • We had a few false start feature PRs that were closed because we took a different direction for the feature area.

More of the suggestions relate to removing that friction.

Suggestions

The first item is already addressed; we have PRs for the C# 8.0 features.

Suggestion: Use LangVersion to improve test fidelity.

The second items suggests one small change. I propose we update our test harness to validate programs by specifying the <LangVersion> element to match the C# version we're working on. (In this case, C# 8.0). That would help us spot when we accidentally spec a feature that comes in a future version. Note: <LangVersion> isn't perfect, but it's very close. I propose this change in the Pre-C# 8.0 milestone.

Suggestion: We review each of the draft V8 PRs in the early C# 8 cycle.

We made significant progress quickly by merging the large PRs, and creating issues to address the remaining comments in additional PRs. (Some of those are in the Pre-C# 8 milestone.)

The following workflow may make us more efficient:

  • Assign a few of the C# 8 feature PRs for initial review in each meeting. At that meeting make one of the following determinations:
    • Decide to spec the feature in a different manner.
    • PR is in the right direction, but major areas are needed before merging.
    • PR is close. Assign to next meeting for 2nd review to address remaining issues.
  • If we decide to spec the features in a different manner, we close the existing PR, and assign someone to make a new PR.
  • If PR is in the right direction, make notes of action items and schedule next review. (At the next review, it should be "close").
  • After a 2nd review, we make any quick updates, create issues for remaining changes, and merge.

The reason for these ideas is to get more of the features merged, even if not perfect. That gives us more flexibility in many ways:

  • We can assign remaining issues to others more easily. The "skeleton" of the feature is merged into the C# 8 branch, so anyone can create new PRs.
  • We have clauses to link to for related features sooner.
  • We recognize when a reorganization of the spec will be beneficial. See Consider reordering and reorganizing sections #959 for one proposal that came very late in the C# 7 timeframe.
@BillWagner
Copy link
Member Author

@jskeet Here's the basic outline for my thoughts on some process updates for C# 8. It covers a bit of 4.3 and 4.4 in the agenda.

@jskeet jskeet added the meeting: discuss This issue should be discussed at the next TC49-TG2 meeting label Oct 2, 2023
@jskeet
Copy link
Contributor

jskeet commented Oct 2, 2023

In terms of using LangVersion, we'll need to check whether we're relying on later-than-C#-8 features like top-level statements or global using directives - but that's fixable, of course.

Proposed workflow sounds feasible - let's discuss it in the meeting.

@jskeet
Copy link
Contributor

jskeet commented Oct 3, 2023

LangVersion: I've just tried setting LangVersion to 8.0 in the projects - 490 out of 497 tests fail :( Basically we use top-level statements and implicit using directives, neither of which are available in C# 8. That's not to say it's all infeasible though - we can potentially add the "standard" using directives to the templates instead. Not being able to use top-level statements is trickier though. Several examples using the "standalone-console" template could probably use "code-in-main" instead, but I suspect not all could.

For reference, top-level statements were introduced in C# 9, and implicit using directives were introduced in C# 10.

As an experiment, I've tried:

  • Setting all templates to use 8.0 other than standalone-console and standalone-console-without-using (which I've set to 9.0)
  • Removing <ImplicitUsings>enable</ImplicitUsings> from all projects

At that point, we have only 27 failures, many of which I suspect could be fixed by adding a using directive.

Does that sound like an avenue worth pursuing? (We could look at how many standalone-console examples could use code-in-main as well, to increase the number of samples being compiled against the right version...)

@BillWagner
Copy link
Member Author

@jskeet

Re: LangVersion: It sounds like it's more work than it's worth in general. As we work on feature areas where there a numerous changes between 8.0 and the current version, we can make those samples use the target version when we have value. Ref safety features comes to mind. Patterns might be another area, although I think those are fairly well known.

@BillWagner
Copy link
Member Author

Discussion from Oct 4 meeting:

  • Over the next month, we should self-assign ourselves to one or more PRs
  • We generally adopted the concepts above.
  • We agree that NRT and patterns are the major features, and it will be important to drive those features to some level of readiness soon.

One goal for the next meeting will be to make a decision on each PR regarding the general direction of the PR, and close those PRs that we decide are not the direction we intend to take.

We agree on the process going forward, so closing as completed.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
meeting: discuss This issue should be discussed at the next TC49-TG2 meeting
Projects
None yet
Development

No branches or pull requests

2 participants