Skip to content

Add ozans playlist concept exercise #1093

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

Merged

Conversation

kristinaborn
Copy link
Contributor

@SleeplessByte I'd love your thoughts on what I've got so far. It was surprisingly difficult to come up with a story for this one.

Hopefully I followed all the new guidelines correctly 🤞

@SleeplessByte
Copy link
Member

SleeplessByte commented Apr 14, 2021

Sweet. I've invited @junedev to review as well as she has a great mental map of the concept-tree as we want it.

As for the failing CI check, here is how you can resolve it:

Either add a comment containing /sync (which I've done now) or go locally (there are more efficient ways to do this, but I like to take the following steps so I know exactly what's going on) and run the following commands:

First bring your main branch up-to-date:

  1. git fetch upstream
  2. git checkout main
  3. git pull
  4. git checkout add-ozans-playlist-concept-exercise

Then sync it:

  1. git rebase main
  2. npx babel-node scripts/sync
  3. git add exercises/concepts/sets
  4. git commit -m "Sync all files"
  5. git push --force

I will attempt to review it before Friday, but chat me up on Slack if you want faster feedback.

@github-actions
Copy link
Contributor

The "Sync all exercises" action has started running.

@github-actions
Copy link
Contributor

The "Sync all exercises" action has finished running.

@github-actions
Copy link
Contributor

For security reasons, /sync does not trigger CI builds when the PR has been submitted from a fork. If checks were not passing due to code format, trigger a build to make the required checks pass, through one of the following ways:

  • Push an empty commit to this branch: git commit --allow-empty -m "Trigger builds".
  • Close and reopen the PR.
  • Push a regular commit to this branch.

Copy link
Member

@junedev junedev left a comment

Choose a reason for hiding this comment

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

I did not do a line-by-line review yet but from what I could glance this looks like great work. 🙂

I added some general comments that might be helpful.

@kristinaborn kristinaborn force-pushed the add-ozans-playlist-concept-exercise branch from 6787d18 to 156e652 Compare April 25, 2021 17:57
@kristinaborn kristinaborn marked this pull request as ready for review April 25, 2021 18:07
@kristinaborn
Copy link
Contributor Author

Marking this as ready to review, but further suggestions are certainly welcome!

In concept exercises we don't skip tests because students don't have skipped tests on the web editor, so this makes it a similar experience via the CLI. They also don't see the tests on the web editor.
@SleeplessByte
Copy link
Member

/format

@github-actions
Copy link
Contributor

The "Format code" action has started running.

@github-actions
Copy link
Contributor

The "Format code" action has finished running.

@github-actions
Copy link
Contributor

For security reasons, /format does not trigger CI builds when the PR has been submitted from a fork. If checks were not passing due to code format, trigger a build to make the required checks pass, through one of the following ways:

  • Push an empty commit to this branch: git commit --allow-empty -m "Trigger builds".
  • Close and reopen the PR.
  • Push a regular commit to this branch.

@SleeplessByte
Copy link
Member

Thank you @junedev for all the wise words, and thank you @kristinaborn for this exercise. Let's merge it now and if we need changes make it in future PRs.

@SleeplessByte SleeplessByte merged commit 8a3f538 into exercism:main Apr 26, 2021
@SleeplessByte
Copy link
Member

@kristinaborn your exercise doesn't show up yet, likely because sets doesn't exist in config.json as concept. If you can do a second PR to fix that, that'd be great!

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
Projects
None yet
Development

Successfully merging this pull request may close these issues.

4 participants