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

Fix for #27 Unwarranted circular property definition #28

Merged
merged 2 commits into from
Jun 21, 2023

Conversation

psenechal-stingray
Copy link
Contributor

...plus minor fix to avoid side effects on ExpansionBuffer::toString

@psenechal-stingray
Copy link
Contributor Author

I'm not sure I understand correctly.
Do you mean that this isn't worth a PR (i.e. making an affirmation)? Feel free to close the PR then.
Or are you commenting on the fork, suggesting that I create a PR? If so, I already did: here.

@robertotru
Copy link

I meant that is was worth a PR. By the way, I had not time to review it for merging it in my branch. My bad, sorry!

@@ -81,6 +81,19 @@ public void multipleValuesAreResolved()
}

@Test
public void propertyIncludesAnotherPropertyMoreThanOnce()

Choose a reason for hiding this comment

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

To match the previous method, wouldn't multipleOccurrencesOfTheSameValueAreResolved be more appropriate?

Copy link
Contributor Author

@psenechal-stingray psenechal-stingray Sep 6, 2016

Choose a reason for hiding this comment

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

One of my concerns naming this test was choosing a name that would be as descriptive as possible.

I agree that multipleOccurencesOfTheSameValueAreResolved is probably grammatically more akin to the other tests.

Then I would suggest multipleOccurencesOfTheSamePropertyWithinAnotherAreResolved ormultipleOccurencesOfTheSamePlaceholderWithinAnotherAreResolved as the terms "property" and "placeholder" seem to be used interchangeably within the file, and the thing being resolved to the value is the property/placeholder.

It's hard to name things right, and I do not really have a strong opinion in this case. I'd let the final word on this to @robertotru.bertotru

@lpearson05
Copy link

@psenechal-stingray If I'm not mistaken, that link ...points to here. I'm confused.

Also, shouldn't there be a test for not a circular reference (e.g., referencingAResolvedPropertyDoesNotThrow), since ...the issue is that this exception is being thrown when we don't expect it.

@psenechal-stingray
Copy link
Contributor Author

psenechal-stingray commented Sep 6, 2016

@lpearson05 I'm a bit confused too. As you've probably seen, I do not contribute frequently on GitHub. It looks like comments on the commit of my fork are also shown here, but not the other way around.

As for referencingAresolvedPropertyDoesNotThrow, isn't it already implicitly tested by the other tests? I cannot think of a test that would not be a duplicate of one or more other test. Please let me know if I'm missing something.

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.

Cannot define one property key more than once Unwarranted "Circular property definition"
4 participants