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

Improve docs around new diagnostics #15330

Closed
wants to merge 4 commits into from
Closed

Conversation

Smaug123
Copy link
Contributor

@Smaug123 Smaug123 commented Jun 6, 2023

Rendered.

There are actually not all that many places which adhere to this, which is why I've linked to some imperfect PRs as examples.

@Smaug123 Smaug123 requested a review from a team as a code owner June 6, 2023 21:04
docs/diagnostics.md Outdated Show resolved Hide resolved
@vzarytovskii
Copy link
Member

It feelks like it either belongs to or needs to be linked from the DEVGUIDE.md, wdyt @Smaug123?

@Smaug123
Copy link
Contributor Author

I guess linking to the diags page from the dev guide is reasonable!

Copy link
Member

@psfinaki psfinaki left a comment

Choose a reason for hiding this comment

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

Overall looks good, please add the link to the dev guide and consider my suggestions below - anyway, thanks!


1. Create a pull request that reserves an error message. (That way, you can avoid other people trampling over your change to FSComp.txt while you're implementing the actual feature.) [Here's a historical example.](https://github.com/dotnet/fsharp/pull/14642)
1. In the same pull request, reserve the new feature. [Here's a historical example](https://github.com/dotnet/fsharp/pull/15315/) (although it would have been better practice to combine this into the previous pull request, and also this pull request [should not have registered the feature](https://github.com/dotnet/fsharp/pull/15315/files#r1220330247) as belonging to a particular language version).
1. Now you can go ahead and implement the diagnostic. This is the point at which you would register the language feature in some particular language version (likely the preview language version). It is often best practice to introduce it at a lower level than you ultimately intend (e.g. informational rather than warning, or warning rather than error), to permit a more gradual rollout. See the section on "[Enabling a warning or error by default](#enabling-a-warning-or-error-by-default)" for more information.
Copy link
Member

Choose a reason for hiding this comment

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

Hm from what I understand, the above PR examples are not entirely "canonical" (as in - things could be organized in a better way) hence the whole thing sounds a bit confusing. I'd suggest rather one of the following:

  1. Finding more canonical examples
  2. Removing them at all and adding once such examples appear
  3. Drawing some picture/diagram/whatever

Copy link
Member

@KevinRansom KevinRansom Jun 16, 2023

Choose a reason for hiding this comment

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

  1. Create a pull request that reserves an error message. (That way, you can avoid other people trampling over your change to FSComp.txt while you're implementing the actual feature.) Here's a historical example.
  2. In the same pull request, reserve the new feature. Here's a historical example (although it would have been better practice to combine this into the previous pull request, and also this pull request should not have registered the feature as belonging to a particular language version).

No, please don't suggest reserving error numbers or pre-registering features prior to implementing the feature. The checkin for the feature is the first place these should arrive in the codebase. Many features, spend months, some even spend years before being completed, or are closed incompleted. We definitely do not want orphaned partial changes checked in to the codebase.

Being able to manage merges and conflicts is a useful skill for developers to have.

@KevinRansom KevinRansom enabled auto-merge (squash) June 16, 2023 17:36
Copy link
Member

@KevinRansom KevinRansom left a comment

Choose a reason for hiding this comment

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

See comment.

@0101
Copy link
Contributor

0101 commented Aug 14, 2023

At this point we don't want to encourage people to split features into multiple PRs.

@0101 0101 closed this Aug 14, 2023
auto-merge was automatically disabled August 14, 2023 17:07

Pull request was closed

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
Archived in project
Development

Successfully merging this pull request may close these issues.

5 participants