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

Add Knapsack exercise #768

Merged
merged 4 commits into from
Sep 14, 2024
Merged

Add Knapsack exercise #768

merged 4 commits into from
Sep 14, 2024

Conversation

kahgoh
Copy link
Member

@kahgoh kahgoh commented Sep 10, 2024

This is one of the 48in24 exercises.

Copy link
Contributor

Hello 👋 Thanks for your PR.

This repo does not currently have dedicated maintainers. Our cross-track maintainers team will attempt to review and merge your PR, but it will likely take longer for your PR to be reviewed.

If you enjoy contributing to Exercism and have a track-record of doing so successfully, you might like to become an Exercism maintainer for this track.

Please feel free to ask any questions, or chat to us about anything to do with this PR or the reviewing process on the Exercism forum.

(cc @exercism/cross-track-maintainers)

config.json Outdated
Copy link
Member

Choose a reason for hiding this comment

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

Formating this file seems like it is a large part of this PR. Could you split that into it's own PR?

Copy link
Member Author

Choose a reason for hiding this comment

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

@IsaacG I've reverted the formatting changes from this PR. I'll be happy to fix the formatting in a follow up PR, after this one.

@IsaacG
Copy link
Member

IsaacG commented Sep 11, 2024

  • @exercism/swift for review.

// Represents an item in knapsack.
// There is be no need to edit this file and changes to this file are ignored by the Exercism test runners.

struct Item {
Copy link
Member

Choose a reason for hiding this comment

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

The indent should be 2 not 4 spaces. Otherwise lgtm

Copy link
Member Author

Choose a reason for hiding this comment

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

I've just updated the exercise source files to use 2 spaces for indents. I wasn't sure about changing the indenting in the test template though. The test templates in the other exercises seem to use 4 spaces, although the formatter fixes the generated test to use 2 spaces. Let me know if the test template should also use 2 spaces.

Copy link
Member

Choose a reason for hiding this comment

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

I'm not part of the Swift team but it sounds like the templates might benefit from an update. If they do get an update, that should probably be a different PR.

Copy link
Member

@meatball133 meatball133 Sep 14, 2024

Choose a reason for hiding this comment

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

The track was a bit of a mess when I took over it tbh, the concepts used 2 indent while the practice exercises used 4 (the online editor used 4). Swift doesn't seem to agree on the indent size. The Swift book (written by apple) uses 4 indent, but if you look at libraries apple writes they use 2 (an example for reference here), I also belive the formatter by default uses 2. Thereby there could have been a transition error when I decided to transition the track to 2 indent. You are free to update the templates files but I don't see it as a necessity

Copy link
Member

Choose a reason for hiding this comment

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

Or actually, Swift 6 includes a new testing framework so that means that we should likely transition to that at some point which means re-writting all test files so likely the easiest thing would be to just apply the fix then.

This is to be consistent with the other exercises that already use 2
spaces.
@meatball133 meatball133 merged commit 8bd0b2f into exercism:main Sep 14, 2024
4 checks passed
@kahgoh kahgoh deleted the exercise/knapsack branch September 17, 2024 21:48
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