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

Expose FSharpProjectSnapshot data. #16716

Merged
merged 2 commits into from
Feb 15, 2024

Conversation

nojaf
Copy link
Contributor

@nojaf nojaf commented Feb 15, 2024

Description

In the new world of snapshots, it is currently impossible to update a snapshot when, for example, the OtherOptions changed. I'd like to expose more data so the static Create method can be used to create an updated version.

This currently is possible with FSharpProjectOptions. I would need this to have parity.

Who do you think @0101?

Checklist

  • Test cases added

  • Performance benchmarks added in case of performance changes

  • Release notes entry updated:

    Please make sure to add an entry with short succinct description of the change as well as link to this pull request to the respective release notes file, if applicable.

    Release notes files:

    • If anything under src/Compiler has been changed, please make sure to make an entry in docs/release-notes/.FSharp.Compiler.Service/<version>.md, where <version> is usually "highest" one, e.g. 42.8.200
    • If language feature was added (i.e. LanguageFeatures.fsi was changed), please add it to docs/releae-notes/.Language/preview.md
    • If a change to FSharp.Core was made, please make sure to edit docs/release-notes/.FSharp.Core/<version>.md where version is "highest" one, e.g. 8.0.200.

    Information about the release notes entries format can be found in the documentation.
    Example:

    If you believe that release notes are not necessary for this PR, please add NO_RELEASE_NOTES label to the pull request.

@nojaf nojaf requested a review from a team as a code owner February 15, 2024 09:56
Copy link
Contributor

github-actions bot commented Feb 15, 2024

⚠️ Release notes required, but author opted out

Warning

Author opted out of release notes, check is disabled for this pull request.
cc @dotnet/fsharp-team-msft

@nojaf nojaf added the NO_RELEASE_NOTES Label for pull requests which signals, that user opted-out of providing release notes label Feb 15, 2024
@0101
Copy link
Contributor

0101 commented Feb 15, 2024

Yeah I think that's fine. Alternatively we could maybe add a Update method which you would call on an existing snapshot with any of these and you'd get a new one.

@vzarytovskii
Copy link
Member

I'm not a huge fan of exposing underlying data types tbh. The more data types we expose, the harder it's going to be to change it. Don't we want to instead expose operations on snapshots/checker for needed actions, so it's easier to deprecate and change it?

@vzarytovskii
Copy link
Member

Nvm, if we want to eventually replace options with snapshot, it's fine

@auduchinok
Copy link
Member

Don't we want to instead expose operations on snapshots/checker for needed actions, so it's easier to deprecate and change it?

It could be good, yes. We don't mind changing the API calls when such public APIs appear. However, we need to start testing the transparent compiler earlier than that happens, so for now let's make it possible for all editors to use and test it, without resorting to adding more IVTs to FCS.

@auduchinok
Copy link
Member

auduchinok commented Feb 15, 2024

Nvm, if we want to eventually replace options with snapshot, it's fine

Yes, it sounds like it's where everyone is going to migrate to eventually when transparent compiler is stable.

Copy link
Contributor

@0101 0101 left a comment

Choose a reason for hiding this comment

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

We can expose these now. But eventually the code that will be creating new snapshots based on updates should also be here so everyone doesn't have to re-implement it.

@nojaf
Copy link
Contributor Author

nojaf commented Feb 15, 2024

Alternatively we could maybe add a Update method which you would call on an existing snapshot with any of these and you'd get a new one.

Yes, perhaps, but something that would fit all use cases would be more ideal to start with.

@psfinaki psfinaki enabled auto-merge (squash) February 15, 2024 12:48
@psfinaki psfinaki merged commit 7c984f0 into dotnet:main Feb 15, 2024
29 checks passed
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
NO_RELEASE_NOTES Label for pull requests which signals, that user opted-out of providing release notes
Projects
Archived in project
Development

Successfully merging this pull request may close these issues.

5 participants