Skip to content

[V3] Freelancer Rates: fix everything around this exercise #1021

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
Merged
SleeplessByte merged 16 commits into from
Mar 12, 2021
Merged

[V3] Freelancer Rates: fix everything around this exercise #1021

SleeplessByte merged 16 commits into from
Mar 12, 2021

Conversation

@SleeplessByte
Copy link
Member

@SleeplessByte SleeplessByte commented Feb 28, 2021

Reviewers, please spell/grammar check

Related #1017
Follow up: #951 (no separate issue created as of opening this PR)

Changes

  • Rename from concept based exercise (numbers) to story based exercise freelancer-rates
  • Add full name to config.json
  • Add blurb for numbers to config.json
  • Add a small welcome block to the stub
  • Add design document
  • Sync concept documents with exercise documents
  • Add concept links
  • Add about.md for numbers concept

@SleeplessByte SleeplessByte added enhancement 🦄 Changing current behaviour, enhancing what's already there documentation 📖 Documentation changes chore 🔧 Meta related task such as build, test, linting, maintainers.json etc. v3-migration 🤖 Preparing for Exercism v3 labels Feb 28, 2021
@SleeplessByte SleeplessByte requested a review from a team February 28, 2021 23:45
@SleeplessByte SleeplessByte mentioned this pull request Feb 28, 2021
Closed
10 tasks
@SleeplessByte SleeplessByte self-assigned this Feb 28, 2021
@SleeplessByte SleeplessByte mentioned this pull request Mar 1, 2021
Closed
@junedev

This comment has been minimized.

Sign in to view
@SleeplessByte

This comment has been minimized.

Sign in to view
@neenjaw

This comment has been minimized.

Sign in to view
neenjaw
neenjaw approved these changes Mar 2, 2021
View reviewed changes
Copy link
Contributor

@neenjaw neenjaw left a comment

Choose a reason for hiding this comment

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

🎉 I think this is overall a good improvement from the last iteration. Left a couple notes about maybe some sentence changes. I was confused with the one table in the about.md.

kotp
kotp reviewed Mar 2, 2021
View reviewed changes
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.

Just observations.

@junedev
Copy link
Member

junedev commented Mar 2, 2021

Thanks for your feedback regarding the purpose of about.md. I will pick up that general discussion in Slack, I don't think this PR is the right place for it.

ErikSchierboom
ErikSchierboom approved these changes Mar 2, 2021
View reviewed changes
exercises/concept/freelancer-rates/.docs/hints.md Outdated

- There is a global built-in function to _parse_ a `string` to a fractional
number, ignoring non-numeric characters, such as the `%` (percent)-sign.
- There is a global built-in function to _parse_ a `string` to a fractional number, ignoring non-numeric characters, such as the `%` (percent)-sign.
Copy link
Member

@ErikSchierboom ErikSchierboom Mar 2, 2021

Choose a reason for hiding this comment

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

A suggestion: you could consider linking to the function.

Copy link
Member Author

@SleeplessByte SleeplessByte Mar 2, 2021

Choose a reason for hiding this comment

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

@junedev suggested removing this task / rewriting it so it doesn't need parsing (cyclic concept dependency), and I like that idea a lot, so this will go away anyway :P :P

Copy link
Member

@ErikSchierboom ErikSchierboom Mar 2, 2021

Choose a reason for hiding this comment

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

👍

@junedev junedev mentioned this pull request Mar 2, 2021
Merged
@SleeplessByte SleeplessByte added the do not merge 🚧 Don't merge until this label is removed label Mar 2, 2021
@SleeplessByte
Copy link
Member Author

SleeplessByte commented Mar 2, 2021

I'm going to update this PR to take into account @junedev 's commentary, and the reviews here. Thanks everyone, this is great stuff!

@SleeplessByte
Copy link
Member Author

SleeplessByte commented Mar 12, 2021

I'm going to merge this and suggest that we make improvements using future issues and PRs.

@SleeplessByte SleeplessByte merged commit 6a30738 into main Mar 12, 2021
@SleeplessByte SleeplessByte deleted the chore/rename-numbers branch March 12, 2021 17:28
@junedev
Copy link
Member

junedev commented Mar 12, 2021

👍 If it is ok for you I would give the exercise/concept another look when I am doing the round of creating the improvement issues.

@SleeplessByte
Copy link
Member Author

SleeplessByte commented Mar 14, 2021

Of course! I hope you will :D and I will be happy to take the advice and implement it :)

This was referenced Jul 14, 2022
Merged
Closed
Closed
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Reviewers

@ErikSchierboom ErikSchierboom ErikSchierboom approved these changes

+2 more reviewers

@kotp kotp kotp left review comments

@neenjaw neenjaw neenjaw approved these changes

Reviewers whose approvals may not affect merge requirements

Assignees

@SleeplessByte SleeplessByte

Labels

chore 🔧 Meta related task such as build, test, linting, maintainers.json etc. do not merge 🚧 Don't merge until this label is removed documentation 📖 Documentation changes enhancement 🦄 Changing current behaviour, enhancing what's already there v3-migration 🤖 Preparing for Exercism v3 x:size/large Large amount of work

Projects

None yet

Milestone

No milestone

Development

Successfully merging this pull request may close these issues.

6 participants

@SleeplessByte @junedev @neenjaw @kotp @ErikSchierboom