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

use case insensitive comparison when searching for dependencies between fable packages #3835

Conversation

ThisFunctionalTom
Copy link
Contributor

@ThisFunctionalTom ThisFunctionalTom commented Jun 9, 2024

This should fix the Issue 3833

@MangelMaxime
Copy link
Member

MangelMaxime commented Jun 9, 2024

/run fantomas

Copy link

github-actions bot commented Jun 9, 2024

@MangelMaxime
Copy link
Member

/run fantomas

@ThisFunctionalTom ThisFunctionalTom force-pushed the fix/fable-packages-compilation-order branch from d16b52d to d1c418f Compare June 9, 2024 13:11
Copy link

github-actions bot commented Jun 9, 2024

Failed to run fantomas: https://github.com/fable-compiler/Fable/actions/runs/9436844381

@MangelMaxime
Copy link
Member

@ThisFunctionalTom Where you able to test it using a locally build version of Fable (via ./build.sh package)?

Running ./build.sh package will give you instruction on how to use that local version of Fable. I am asking, because this fix is not something we can catch easily using the CI I think.

@ThisFunctionalTom
Copy link
Contributor Author

ThisFunctionalTom commented Jun 9, 2024

@ThisFunctionalTom Where you able to test it using a locally build version of Fable (via ./build.sh package)?

Running ./build.sh package will give you instruction on how to use that local version of Fable. I am asking, because this fix is not something we can catch easily using the CI I think.

I debuged fable.cli on my project with a breakpoint and looked at the file . I thought about adding tests for sortFablePackages which shouldn't be that hard because it is a pure function but I didn't find the right place where to put the tests. Do you have an idea where should I put the tests?

I will try running it with build.sh later but I would probably need to use build.cmd because I am on windows.

@MangelMaxime
Copy link
Member

MangelMaxime commented Jun 9, 2024

I will try running it with build.sh later but I would probably need to use build.cmd because I am on windows.

Yes should be the same.

Do you have an idea where should I put the tests?

Perhaps the tests/Integration/Integration/Fable.Tests.Integration.fsproj project could be the place for it? It seems like we are testing some internal APIs from Fable in it.

Or if we need to full project setup to test it, then we can create a new project under tests/Integration/ProjectConfigs and compile / run it here

testProjectConfig "DebugWithExtraDefines" (Some "Debug")
testProjectConfig "CustomConfiguration" (Some "Test")
testProjectConfig "ReleaseNoExtraDefines" None
testProjectConfig "ConsoleApp" None

I think if the first one works then this is enough.

@ThisFunctionalTom
Copy link
Contributor Author

ThisFunctionalTom commented Jun 9, 2024

Oh, I cannot test it because the function sortFablePackages and type FablePackage is not public (they are not in the fsi file).

In our projects we make testable functions internal and add [assembly:InternalsVisibleTo("MyTestAssembly")] to expose internal functions and types to test projects.

Should I do this? Is this compatible with fable repository coding style? Is there another way to test private functions?

@ncave
Copy link
Collaborator

ncave commented Jun 9, 2024

I think it's fine as proposed, the spec says that package ids are case-insensitive.

@MangelMaxime
Copy link
Member

Let's keep it simple and merge the PR as is.

@MangelMaxime MangelMaxime merged commit c13d8ad into fable-compiler:main Jun 10, 2024
18 checks passed
@MangelMaxime
Copy link
Member

This has been released

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.

3 participants