Skip to content

Conversation

@eaba
Copy link
Contributor

@eaba eaba commented Apr 12, 2021

Make sure you have checked all steps below.

Jira

Tests

Commits

  • My commits all reference Jira issues in their subject lines. In addition, my commits follow the guidelines from "How to write a good git commit message":
    1. Subject is separated from body by a blank line
    2. Subject is limited to 50 characters (not including Jira issue reference)
    3. Subject does not end with a period
    4. Subject uses the imperative mood ("add", not "adding")
    5. Body wraps at 72 characters
    6. Body explains "what" and "why", not "how"

Copy link
Member

@blachniet blachniet left a comment

Choose a reason for hiding this comment

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

Do you think you could add a unit test that verifies this fix?

In other words, a test that fails before the changes, but passes afterwards.

Comment on lines 218 to 220
if (!_previousFields.ContainsKey(f.Name))
{
_previousFields.TryAdd(f.Name, f.Schema);
Copy link
Member

Choose a reason for hiding this comment

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

I believe you can collapse the omit the ContainsKey by checking the return value of TryAdd. The docs for TryAdd state that it returns:

true if the key/value pair was added to the ConcurrentDictionary<TKey,TValue> successfully; false if the key already exists.

So, maybe you can do something like this:

Suggested change
if (!_previousFields.ContainsKey(f.Name))
{
_previousFields.TryAdd(f.Name, f.Schema);
if (_previousFields.TryAdd(f.Name, f.Schema);)
{

*  check the return value of TryAdd
@eaba
Copy link
Contributor Author

eaba commented May 9, 2021

Do you think you could add a unit test that verifies this fix?

In other words, a test that fails before the changes, but passes afterwards.

@blachniet I have added test case

@dkulp dkulp merged commit 62a2d3f into apache:master Sep 13, 2021
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Labels

Projects

None yet

Development

Successfully merging this pull request may close these issues.

3 participants