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

Build concept exercises + fix concept exercise that does not build. #915

Merged
merged 12 commits into from
Oct 16, 2024

Conversation

heijp06
Copy link
Contributor

@heijp06 heijp06 commented Oct 10, 2024

I wanted to fix issue #878, but noticed that the doctor-data exercise was not built or tested when the track was built. It turned out that this was the case for all concept exercises. I am assuming this is NOT by design.

I did the following:

  • Change the build such that it builds both "concept" and "practice" exercises.
  • Fix a concept exercise freelancer-rates that did not build.
  • Fix a typo in the text that comes with the freelancer-rates exercise.

freelancer-rates failed to build because of the following line of code:

int rounded_up{std::ceil(after_discount)};

Narrowing conversions (ceil() returns a double) are not allowed with list-initialization.

I tested the build of the track with cmake . && make on Ubuntu 22.04.

Copy link
Contributor

Hello. Thanks for opening a PR on Exercism 🙂

We ask that all changes to Exercism are discussed on our Community Forum before being opened on GitHub. To enforce this, we automatically close all PRs that are submitted. That doesn't mean your PR is rejected but that we want the initial discussion about it to happen on our forum where a wide range of key contributors across the Exercism ecosystem can weigh in.

You can use this link to copy this into a new topic on the forum. If we decide the PR is appropriate, we'll reopen it and continue with it, so please don't delete your local branch.

If you're interested in learning more about this auto-responder, please read this blog post.


Note: If this PR has been pre-approved, please link back to this PR on the forum thread and a maintainer or staff member will reopen it.

@github-actions github-actions bot closed this Oct 10, 2024
@iHiD iHiD reopened this Oct 11, 2024
@heijp06
Copy link
Contributor Author

heijp06 commented Oct 11, 2024

@vaeng can you review this?

@vaeng
Copy link
Contributor

vaeng commented Oct 11, 2024

Thank you for your PR.

My CMake-foo is not the best, I would prefer @ahans or @siebenschlaefer to look at your changes.

Please do not mix different issues into one PR. It is easier to handle pull requests if they only concern one problem at a time.
E.g. one for the docs, one for the build pipeline, and one for the non-compiling exercise (that has a fix in pr #916 with an added comment, because I have not seen your PR earlier).

@heijp06
Copy link
Contributor Author

heijp06 commented Oct 11, 2024

@vaeng I reverted my change to the docs and created a separate PR for that: #918 Can you open that?

The change in the non-compiling exercise is not really unrelated to the fix in the pipeline, in that the pipeline can only be fixed if all exercises compile. So I will keep that as is for now. If your PR completes before mine I can fix that by merging.

CI is failing on Windows, I will look into that.

@vaeng vaeng added x:type/ci Work on Continuous Integration (e.g. GitHub Actions workflows) hacktoberfest-accepted labels Oct 16, 2024
@vaeng vaeng merged commit 3d106a5 into exercism:main Oct 16, 2024
8 checks passed
@heijp06 heijp06 deleted the test_concept_exercises branch October 17, 2024 18:30
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
x:type/ci Work on Continuous Integration (e.g. GitHub Actions workflows)
Projects
None yet
Development

Successfully merging this pull request may close these issues.

3 participants