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

Took some fixes from FSharp.TypeProvider.SDK to TestTP. #18252

Merged
merged 5 commits into from
Jan 23, 2025

Conversation

Thorium
Copy link
Contributor

@Thorium Thorium commented Jan 20, 2025

I took some separate changes from fsprojects/FSharp.TypeProvider.SDK to TestTP.
The goal would be to get these files as close to each other as possible.
After that, you can discuss whether TPs should be maintained in the F# compiler repo or not, if you like.

This file is only changing tests, so no production changes should appear.
Thus no readme changes or release notes.

Remarks:

  • FSharp.TypeProvider.SDK has lazy static parameters, which are not included here. It would probably make sense to include them here; I don't know why they are used, but there is probably a reason. (They can be added later.)
  • FSharp.TypeProvider.SDK has some hacks for reference assembly selection that are not included here. They might be important, or they might be just old compatibility things; I don't know.
  • FSharp.TypeProvider.SDK has some F# 3.1 compatibility and other compiler flags that are not included in the current F# compiler project, so there is no reason to port those, in my opinion.
  • ProvidedTypes.fs and ProvidedTypes.fsi are included in the dotnet/fsharp test projects in three separate times. Only TestTP seems to be included in the solution, the others are some kind of other integration tests. They seem to be living a little bit of their own life; all are a bit different. Considering that ProvidedTypes.fs is like 800kB size, it could make sense to have the file only once, and then include the file as link to other places.

@Thorium Thorium requested a review from a team as a code owner January 20, 2025 18:13
Copy link
Contributor

✅ No release notes required

@T-Gro
Copy link
Member

T-Gro commented Jan 23, 2025

As real TPs can also use older versions of ProvidedTypes, I think it is good that tests include different variations - to increase coverage.
That is for compatibility with different versions of ProvidedTypes.fs that TPs might be using.
When it comes to compatibility with older compilers, you are right - this is not needed, is it won't be exercised by the test suite anyway.

@T-Gro T-Gro enabled auto-merge (squash) January 23, 2025 10:14
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.

Thanks!

@psfinaki
Copy link
Member

/azp run

Copy link

Azure Pipelines successfully started running 2 pipeline(s).

@T-Gro T-Gro merged commit 2d6b138 into dotnet:main Jan 23, 2025
33 checks passed
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.

3 participants