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

Fix Dictionary formatting, and cleanup #9

Merged
merged 3 commits into from Oct 25, 2017

Conversation

Projects
None yet
2 participants
@bsinky
Collaborator

bsinky commented Oct 24, 2017

Addresses the code review comments from #4. Since that pull request is merged and closed, I don't believe I could have updated it...?

Also noticed the comments and default messages between ArgumentException and the newly added ArgumentNullException were inconsistent, took the opportunity to clean those up.

@bsinky

This comment has been minimized.

Collaborator

bsinky commented Oct 25, 2017

I noticed Assert.throws was added recently, so I took the opportunity to tidy up DictionaryTests a bit.

@nightblade9

This comment has been minimized.

Owner

nightblade9 commented Oct 25, 2017

Yes, the lack of Assert.throws has been annoying me for some time. There's an implementation in the (currently unreleased) source version of munit. Maybe we should switch to munit from source; the last release was two years ago.

{
//Pass
}
Assert.throws(KeyNotFoundException, (_) => testKeyNotFoundDictionary.get(2));

This comment has been minimized.

@nightblade9

nightblade9 Oct 25, 2017

Owner

I was wondering about this. Does this work with () => instead of (_)? What exactly does the underscore represent?

This comment has been minimized.

@bsinky

bsinky Oct 25, 2017

Collaborator

With hxslam, it does not: https://github.com/bynuff/hxslam#usage

This comment has been minimized.

@nightblade9

nightblade9 Oct 25, 2017

Owner

That explains why my other tests weren't compiling. Thanks for figuring this out.

public override function set(key:K, value:V)
/**
Returns the number of elements in the Dictionary
Note that this is an O(N) operation

This comment has been minimized.

@nightblade9

nightblade9 Oct 25, 2017

Owner

+1 for mentioning the runtime

@nightblade9 nightblade9 merged commit 68d8987 into nightblade9:master Oct 25, 2017

1 check passed

continuous-integration/travis-ci/pr The Travis CI build passed
Details

@bsinky bsinky deleted the bsinky:add-dictionary branch Oct 25, 2017

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment