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

Paket Simplify #1204

Merged
merged 1 commit into from
Nov 16, 2023
Merged

Paket Simplify #1204

merged 1 commit into from
Nov 16, 2023

Conversation

1eyewonder
Copy link
Contributor

@1eyewonder 1eyewonder commented Nov 15, 2023

WHAT

🤖 Generated by Copilot at 6eac6bb

This pull request simplifies and cleans up the dependency management of the FsAutoComplete solution by removing unused or redundant references from the paket.dependencies file and various paket.references files. It also adds some new references for code analysis and performance optimization in the core library and the main executable projects. Additionally, it reorganizes the build and benchmark dependencies to share a common paket.references file.

🤖 Generated by Copilot at 6eac6bb

paket.references
Simplified and updated
A fresh spring cleaning

🧹🚚➕

WHY

Simple code cleanup for the paket dependencies and references

HOW

🤖 Generated by Copilot at 6eac6bb

  • Remove unused references from various projects (link, link, link, link, link, link, link, link, link, link, link)
  • Add references for code analysis and performance optimization to the core library and main executable projects (link, link)
  • Add references for testing project loading and resolution to the LSP test project (link)
  • Move the build dependencies to the benchmarks/paket.references file, which is shared with the benchmark projects (link)
  • Remove the empty dependencies group from the paket.dependencies file (link)

@@ -1,4 +1,3 @@
FSharp.Core
Copy link
Contributor

Choose a reason for hiding this comment

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

Are you sure we want to remove this entry?
Don't we want to have the explicit FSharp.Core reference?

Copy link
Contributor Author

Choose a reason for hiding this comment

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

I can add it back after work. I made the assumption paket knew better than me. Silly question but why would we need to keep it if paket is saying it is 'unused'? Is this a paket bug you think?

Copy link
Contributor

Choose a reason for hiding this comment

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

Hmm, that sounds a bit weird to me. Isn't F# Core always used? The compiler needs it to compile your code right?

Copy link
Member

Choose a reason for hiding this comment

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

it's marked as a transitive dependency. I know project files have some special rules around FSharp.Core (implicit vs explicit). Probably just need to look at project.assets.json and see if it's using the transitive defined in paket.lock or the sdk version

Choose a reason for hiding this comment

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

@1eyewonder could you log an issue on paket, with a bit of info you have at hand to reproduce it (the more the merrier), I think any paket.references to F# project that references FSharp.Core should have this line in it, and we can move this discussion to paket repository to gather more insight on what should occur.

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Yep, I can get on that later this evening. As for this PR, we are in agreeance to add FSharp.Core back in the project.references correct?

Copy link
Member

Choose a reason for hiding this comment

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

Just check the built output in project assets json and if it’s the one in paket lock and not whatever sdk you have installed it should be fine.

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Left is paket.lock, right is project.assets.json for src/FsAutoComplete
image

Left is paket.lock, right is project.assets.json for benchmarks
image

I think we should be good to proceed

@TheAngryByrd TheAngryByrd merged commit e4c242c into ionide:main Nov 16, 2023
12 of 13 checks passed
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.

4 participants