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

Issue #1226 implemented #1231

Merged
merged 2 commits into from
Aug 11, 2015
Merged

Issue #1226 implemented #1231

merged 2 commits into from
Aug 11, 2015

Conversation

Exef
Copy link
Contributor

@Exef Exef commented Aug 10, 2015

Both Put() and CleanUp() methods in class SimpleDnsCache.Cache are now immutable.

It's my first pull request, so if you find any issues let me know and I'll fix them. Any ideas what unit test should I add?

@Aaronontheweb
Copy link
Member

@Exef need to rebase your branch on dev so we can merge it though! CONTRIBUTING.MD contains a guide that'll explain how to do this.

I haven't reviewed the code yet though - but I'll take a look.

@Exef
Copy link
Contributor Author

Exef commented Aug 11, 2015

@Aaronontheweb Sure, my bad. I'll fix it today. I've read everything that was linked from README.md, but there was no link to CONTRIBUTING.md. I'll open docs issue to fix this.

@rogeralsing
Copy link
Contributor

There is still some mutability going on in the Cleanup method:

             public Cache Cleanup()
             {
                 var now = _clock();
                 while (_queue.Any() && !_queue.First().IsValid(now))
                 {
                     var minEntry = _queue.First();
                     var name = minEntry.Name;
                     _queue.Remove(minEntry);  //<-------------------------------------------- here
                     if (_cache.ContainsKey(name) && !_cache[name].IsValid(now))
                         _cache.Remove(name); //<-------------------------------------------- here
                 }
                 return new Cache(_queue, _cache, _clock);
             }

It does return a new Cache, which is good, but the old Cache is now changed.

@Exef
Copy link
Contributor Author

Exef commented Aug 11, 2015

Yeah, I've missed it. Also putting an element to new Cache also put to old cleaned-up cache.

Both Put() and CleanUp() methods in class SimpleDnsCache.Cache is now
immutable.
@Exef
Copy link
Contributor Author

Exef commented Aug 11, 2015

Ok, done. What else should I do? EDIT: Fix the test.

Changed SimpleDnsCache.Put() to pass
SimpleDnsCacheSpec.Cache_should_be_updated_with_the_latest_resolved
rogeralsing added a commit that referenced this pull request Aug 11, 2015
@rogeralsing rogeralsing merged commit 43fa97d into akkadotnet:dev Aug 11, 2015
@rogeralsing
Copy link
Contributor

👍 Thanks

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.

3 participants