Skip to content

implement error object on multiple exercises#1337

Merged
rpottsoh merged 11 commits intoexercism:masterfrom
rpottsoh:addErrorObjects
Sep 24, 2018
Merged

implement error object on multiple exercises#1337
rpottsoh merged 11 commits intoexercism:masterfrom
rpottsoh:addErrorObjects

Conversation

@rpottsoh
Copy link
Copy Markdown
Member

@rpottsoh rpottsoh commented Sep 21, 2018 •
edited
Loading

per #1311
phone-number
say
variable-length-quantity
queen-attack
forth

@Insti Insti mentioned this pull request Sep 21, 2018
Closed
13 tasks
sdublish
sdublish reviewed Sep 21, 2018
View reviewed changes
Comment thread exercises/say/canonical-data.json Outdated
"version": "1.1.0",
"version": "1.2.0",
"comments": [
"Here -1 is used as expected value to indicate that the",
Copy link
Copy Markdown
Contributor

@sdublish sdublish Sep 21, 2018

Choose a reason for hiding this comment

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

Would it make sense to update the comments to reflect the new error object?

Copy link
Copy Markdown
Member Author

@rpottsoh rpottsoh Sep 23, 2018

Choose a reason for hiding this comment

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

Yes it would. 😄 Thanks for pointing it out.

@Insti
Copy link
Copy Markdown
Contributor

Insti commented Sep 21, 2018 •
edited
Loading

Multiple changes in 1 PR makes reviewing harder. (Note to self: remember to view commits individually.)

Make sure you check all the comments at the start of the files.

Thanks for taking this on @rpottsoh 👍

@rpottsoh
Copy link
Copy Markdown
Member Author

rpottsoh commented Sep 23, 2018

Multiple changes in 1 PR makes reviewing harder.

Sorry about that. I hadn't considered that.

@rpottsoh
Copy link
Copy Markdown
Member Author

rpottsoh commented Sep 23, 2018

Should I just bust this up into separate PRs? This is getting messy, fast.

@Insti
Copy link
Copy Markdown
Contributor

Insti commented Sep 24, 2018

Should I just bust this up into separate PRs? This is getting messy, fast.

As long as nothing else needs to be done it's probably OK, just something to keep in mind for next time.

sdublish
sdublish approved these changes Sep 24, 2018
View reviewed changes
Copy link
Copy Markdown
Contributor

@sdublish sdublish left a comment

Choose a reason for hiding this comment

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

Looks good; thanks for fixing the comments!

@rpottsoh rpottsoh merged commit aaadbac into exercism:master Sep 24, 2018
rpottsoh added a commit that referenced this pull request Sep 24, 2018
@rpottsoh
Merge remote-tracking branch 'origin/addErrorObjects' (#1337)
13da639
@rpottsoh rpottsoh deleted the addErrorObjects branch September 24, 2018 19:02
@pgaspar pgaspar mentioned this pull request Oct 6, 2018
Merged
ZapAnton added a commit to ZapAnton/rust that referenced this pull request Nov 20, 2018
@ZapAnton
phone-number: Updated the exercise to the 1.6.1 version
b007852 
Relevant PRs:
- exercism/problem-specifications#772
- exercism/problem-specifications#1090
- exercism/problem-specifications#1120
- exercism/problem-specifications#1327
- exercism/problem-specifications#1337
- exercism/problem-specifications#1361
@ZapAnton ZapAnton mentioned this pull request Nov 20, 2018
Merged
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Reviewers

2 more reviewers

@Insti Insti Insti approved these changes

@sdublish sdublish sdublish approved these changes

Reviewers whose approvals may not affect merge requirements

Assignees

No one assigned

Labels

None yet

Projects

None yet

Milestone

No milestone

Development

Successfully merging this pull request may close these issues.

3 participants

@rpottsoh @Insti @sdublish