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.

Sign up for GitHub

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

Jump to bottom

Improvements proposition #27

Closed
wants to merge 3 commits into from
Closed

Improvements proposition #27

wants to merge 3 commits into from

Conversation

MangelMaxime
Copy link
Contributor

@MangelMaxime MangelMaxime commented Dec 16, 2023 •
edited
Loading

WHAT

copilot:summary

copilot:poem

copilot:emoji

WHY

HOW

copilot:walkthrough

Sorry for the big PR, the auto formatting didn't help on that here 🙈

I think copilot for PR is not available anymore so I will write a good old description myself ahah.

  • Change Releases from a list of tuple to using a list of record has we are starting to have a lot of properties
  • Disable formatting on save as it makes too many changes and Clean up #24 addresses the formatting setup
  • Fix Support [YANKED] tag #26

I was not able to test the Ionide.KeepAChangelog.Tasks project because when packing it there seems to be missing some dll.

I think this is something that was discovered with the last release already

➜  Thoth.Json.Core git:(feature/kevin_pr) ✗ dotnet pack
MSBuild version 17.8.3+195e7f5a3 for .NET
  Determining projects to restore...
  Restored /Users/mmangel/Workspaces/Github/thoth-org/Thoth.Json/packages/Thoth.Json.Core/Thoth.Json.Core.fsproj (in 800 ms).
/Users/mmangel/.nuget/packages/ionide.keepachangelog.tasks/900.2.0/build/Ionide.KeepAChangelog.Tasks.targets(29,9): error MSB4062: The "Ionide.KeepAChangelog.Tasks.ParseChangeLogs" task could not be loaded from the assembly /Users/mmangel/.nuget/packages/ionide.keepachangelog.tasks/900.2.0/build/../tasks/net6.0/Ionide.KeepAChangelog.Tasks.dll. Could not load file or assembly 'FSharp.Core, Version=6.0.0.0, Culture=neutral, PublicKeyToken=b03f5f7f11d50a3a'. The system cannot find the file specified. [/Users/mmangel/Workspaces/Github/thoth-org/Thoth.Json/packages/Thoth.Json.Core/Thoth.Json.Core.fsproj]
/Users/mmangel/.nuget/packages/ionide.keepachangelog.tasks/900.2.0/build/Ionide.KeepAChangelog.Tasks.targets(29,9): error MSB4062:  Confirm that the <UsingTask> declaration is correct, that the assembly and all its dependencies are available, and that the task contains a public class that implements Microsoft.Build.Framework.ITask. [/Users/mmangel/Workspaces/Github/thoth-org/Thoth.Json/packages/Thoth.Json.Core/Thoth.Json.Core.fsproj]

MangelMaxime
MangelMaxime commented Dec 16, 2023
View reviewed changes
src/Ionide.KeepAChangelog/Library.fs
Comment on lines -247 to +261
let date = pDate .>> skipRestOfLine true
let date = pDate
let yanked = opt spaces1 >>. pYanked
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 was not able to keep the .>> skipRestOfLine true for some reason it was making the tests fails.

@nojaf
Copy link
Contributor

nojaf commented Dec 16, 2023

If a code repository is not set up to use auto-formatting, you should not use a formatter in your PR!
I tried to address this in #24, but as long as the current code is not being checked on every commit you should not use a formatter.

This would have saved you a lot of effort, to be honest 😸

@MangelMaxime
Copy link
Contributor Author

MangelMaxime commented Dec 16, 2023 •
edited
Loading

I agree problem was that it was setup to use it on save and saw it too late.

I will re-send the PR without the formatting change.

@MangelMaxime MangelMaxime force-pushed the feature/various_proposition branch from 56074a5 to 4e72d71 Compare December 16, 2023 16:54
@MangelMaxime
Copy link
Contributor Author

MangelMaxime commented Sep 16, 2024 •
edited
Loading

I am closing because I think going in the direction from #17 makes more sense.

Out sourcing the parser work would make it easier to maintain Ionide.KeepAChangelog.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Reviewers
No reviews
Assignees
No one assigned
Labels
None yet
Projects
None yet
Milestone
No milestone
Development

Successfully merging this pull request may close these issues.

Support [YANKED] tag
2 participants
@MangelMaxime @nojaf