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

Implement etl #27

Merged
merged 7 commits into from
Nov 8, 2015
Merged

Implement etl #27

merged 7 commits into from
Nov 8, 2015

Conversation

sturmer
Copy link

@sturmer sturmer commented Oct 18, 2015

No description provided.

sturmer added 3 commits October 17, 2015 20:15
The example should be working, I need to fix the tests.
@sturmer
Copy link
Author

sturmer commented Oct 18, 2015

Hi everyone, I've just submitted an implementation for ETL. As usual, any comment is welcome.

@sturmer
Copy link
Author

sturmer commented Oct 25, 2015

Dear @yurrriq, @arguello, @bennn, when/if you have time I could use your input on this.


(test-case
"mixed case input"
(let ([actual (etl mixed-case-input)])
Copy link
Contributor

Choose a reason for hiding this comment

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

Could also assert (check-equal? (hash-count actual) (hash-count expected))

Copy link
Author

Choose a reason for hiding this comment

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

Good idea, will do.

sturmer added 2 commits October 27, 2015 07:52
There is some reasoning to be done about the contract in the example,
but the code should be fine.
The input is now checked in tests. To check the output, one can just
change the last line of the example implementation by removing the
application of the function string-downcase to see the contract
failing spectacularly.
@sturmer
Copy link
Author

sturmer commented Oct 28, 2015

@bennn Made some honest contracts working. What do you think?

@bennn
Copy link
Contributor

bennn commented Oct 28, 2015

They're good!

(check-equal? (hash-ref actual k)
(hash-ref expected k))))

(test-true
Copy link
Contributor

Choose a reason for hiding this comment

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

A shorter way is

(test-exn
  "test bad input: negative keys"
  exn:fail:contract?
  (lambda () (etl negative-keys input)))

Copy link
Author

Choose a reason for hiding this comment

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

Thanks for the comments @bennn. That would also get rid of the (lambda _ #t) which I didn't love --- I'll admit it. I am going to make those changes tomorrow.

@sturmer
Copy link
Author

sturmer commented Oct 29, 2015

The contract testing part is now more concise, thanks again to @bennn.

@sturmer
Copy link
Author

sturmer commented Nov 3, 2015

If this is considered good, could anyone please merge it? I think @arguello was so kind to do it last time? Please let me know if there is anything else I can do on my side.

(define (etl h)
(for*/hash ([(score letter*) (in-hash h)]
[letter (in-list letter*)])
;(printf "~a: ~a\n" letter score)
Copy link
Member

Choose a reason for hiding this comment

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

I might suggest deleting this line.

Copy link
Author

Choose a reason for hiding this comment

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

Approved!

@yurrriq
Copy link
Member

yurrriq commented Nov 3, 2015

After a quick once over it LGTM. Dig the contracts.

It was a leftover of debugging mode. Thanks @yurrriq for the tip.
@sturmer
Copy link
Author

sturmer commented Nov 3, 2015

@yurrriq Commented line removed. Thanks for digging the contracts :)

@sturmer
Copy link
Author

sturmer commented Nov 8, 2015

@yurrriq Lest this might escape your radar, a small reminder.

arguello added a commit that referenced this pull request Nov 8, 2015
@arguello arguello merged commit 8c1f129 into exercism:master Nov 8, 2015
@sturmer
Copy link
Author

sturmer commented Nov 8, 2015

Thank you @arguello!

@arguello
Copy link
Contributor

arguello commented Nov 8, 2015

Thank you :)

@yurrriq
Copy link
Member

yurrriq commented Nov 8, 2015

Thanks, guys.

@BNAndras BNAndras mentioned this pull request Sep 7, 2023
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.

4 participants