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 #1644

Merged
merged 12 commits into from
Mar 18, 2024
Merged

Conversation

fpsvogel
Copy link
Contributor

@github-actions github-actions bot closed this Mar 15, 2024
@kotp kotp reopened this Mar 15, 2024
@exercism exercism deleted a comment from github-actions bot Mar 15, 2024
@kotp
Copy link
Member

kotp commented Mar 15, 2024 via email

@kotp
Copy link
Member

kotp commented Mar 15, 2024

About the failing test, it is due to the Data use in the test, which I commented on as something I do like. But we should only use it if it is defined. This will require either a version check in the test with an alternative solution for that structure and the desired behavior, or a choice not to use Data in the test at all.

@kotp
Copy link
Member

kotp commented Mar 15, 2024

This will require either a version check in the test with an alternative solution for that structure and the desired behavior, or a choice not to use Data in the test at all.

Nah, just a conscious decision to bring it in even with the 3.1 version failing, for a known reason. This will pass on the platform, as it is using Ruby 3.3 according to the repository.

@fpsvogel
Copy link
Contributor Author

@kotp Thanks for the feedback! I've made the changes that you suggested. Plus, I added punctuation and fixed typos in the explanation quoted from Reddit, in order to make it more readable.

Copy link
Member

@kotp kotp left a comment

Choose a reason for hiding this comment

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

Looking good, but a few changes are probably warranted in communicating what must be rather than what should be.

@kotp kotp merged commit 7e8509d into exercism:main Mar 18, 2024
3 of 4 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.

2 participants