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

feat(core): add fsharp init-template #1912

Merged
merged 3 commits into from
Mar 12, 2019

Conversation

lucasschejtman
Copy link
Contributor

Add F# init-template to the cli.

I followed the C# patterns, however, the solution file (.sln) doesn't reference the project file (.[cd|fs]proj) which in return doesn't reference the project files. That makes them quite unusable out of the box.

The proposed solutions would be

  1. Add the proper references in the template files
  2. Update the template instructions to include the relevant dotnet add commands.

Happy to create an issue and submit another PR to address it.


Pull Request Checklist

  • Testing
    • Unit test added (prefer not to modify an existing test, otherwise, it's probably a breaking change)
    • CLI change?: coordinate update of integration tests with team
    • cdk-init template change?: coordinated update of integration tests with team
  • Docs
    • jsdocs: All public APIs documented
    • README: README and/or documentation topic updated
  • Title and Description
    • Change type: title prefixed with fix, feat will appear in changelog
    • Title: use lower-case and doesn't end with a period
    • Breaking?: last paragraph: "BREAKING CHANGE: <describe what changed + link for details>"
    • Issues: Indicate issues fixed via: "Fixes #xxx" or "Closes #xxx"
  • Sensitive Modules (requires 2 PR approvers)
    • IAM Policy Document (in @aws-cdk/aws-iam)
    • EC2 Security Groups and ACLs (in @aws-cdk/aws-ec2)
    • Grant APIs (only if not based on official documentation with a reference)

By submitting this pull request, I confirm that my contribution is made under the terms of the Apache-2.0 license.

@lucasschejtman lucasschejtman requested a review from a team as a code owner February 28, 2019 23:00
@eladb
Copy link
Contributor

eladb commented Mar 4, 2019

@costleya can you please take a look here?

@eladb eladb requested a review from costleya March 4, 2019 10:10
@Doug-AWS
Copy link
Contributor

Doug-AWS commented Mar 5, 2019

If F# has any additional or different prerequisites compared to C#, please enumerate them and ping me by adding Doug-AWS to the comment. Here is what I have for C#:

.NET standard 2.0 compatible implementation:

.NET Core >= 2.0
.NET Framework >= 4.6.1
Mono >= 5.4

@lucasschejtman
Copy link
Contributor Author

@Doug-AWS Same requirements, although there are some workarounds at the compiler level for FSI support, F# sits on top on .NET Standard 2.0 since 4.3.1.

@@ -0,0 +1,4 @@
{
"app": "dotnet src/HelloCdk/bin/Debug/netcoreapp2.1/HelloCdk.dll"
Copy link
Contributor

Choose a reason for hiding this comment

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

I'm going to change the C# cdk.json template to:

"app": "dotnet run --project HelloCdk/HelloCdk.csproj"

To eliminate the need to remember to recompile the application every time before you run "cdk *". Feel free to make this change now, or I can make it when I update the C# one.

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'll update it after the C# one.
Do you have an opinion on my comments at the beginning regarding the solution not referencing the project and the project not referencing the project files? They're not very useful in their current state, happy to update those as well.

Copy link
Contributor

Choose a reason for hiding this comment

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

Thanks for calling this out.

The C# template adds the project after generation using the add-project.hook.ts. I see you copied this file, but did not update it for the F# template. Try changing the "dotnet add" command to point to the fsproj, and see if it works. I added the comments to the lines that need to change.

Copy link
Contributor Author

Choose a reason for hiding this comment

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

This still doesn't address the missing project files in the project file (HelloCdk.[c|f]sproj),
could add the logic to the add-project.hook.ts file in another PR.

Also, dotnet restore is not automatically done in all IDEs so it could be added as another post template init logic like we do with npm and maven.

Copy link
Contributor

Choose a reason for hiding this comment

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

I'm confused about what is missing. The add-project hook calls dotnet add. If there are additional resources missing for F# on the project level, then you can either add those references into the template, or you can take care of it in the/a hook.

The dotnet restore is definitely something that could be added.

Copy link
Contributor Author

Choose a reason for hiding this comment

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

@costleya My bad, forgot they changed it in the last version of the framework, you don't need to add files to project now. It's directory based now.

Happy to add the dotnet restore bit in a separate PR to not pollute this one.

Copy link
Contributor

Choose a reason for hiding this comment

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

Sounds like a plan.

@@ -0,0 +1,4 @@
{
"app": "dotnet src/HelloCdk/bin/Debug/netcoreapp2.1/HelloCdk.dll"
Copy link
Contributor

Choose a reason for hiding this comment

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

Thanks for calling this out.

The C# template adds the project after generation using the add-project.hook.ts. I see you copied this file, but did not update it for the F# template. Try changing the "dotnet add" command to point to the fsproj, and see if it works. I added the comments to the lines that need to change.

@eladb eladb merged commit dfefb58 into aws:master Mar 12, 2019
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