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

enhance error message for circular references #50

Merged
merged 3 commits into from
Sep 23, 2020

Conversation

sriv
Copy link
Contributor

@sriv sriv commented Sep 23, 2020

Note: This is an improvement over #49 for circular reference error message.

Context

When there are properties with circular references, the error message thrown contains one of the key in the cycle. This can be a bit annoying to fix, since it does not give a trace. Given this library supports loading properties from multiple sources, grep etc can fail.

Input:

foo = depends on ${bar}

bar = which depends on ${baz}

baz = ${foo} completes a cyclic reference

The error message thrown when properties.MustLoadFile is called with the above file:

circular reference in "foo = ${foo}"

This pull request is an attempt to make it look better, my take:

circular reference in:
foo=depends on ${bar}
bar=which depends on ${baz}
baz=${foo} completes a cyclic reference

This is not perfect, it still does not highlight the exact source of these properties, but I felt that the overhead of carrying that context to properties.expand may be a bit too much, since the method is currently stateless.

Thanks for the library!

Signed-off-by: sriv <srikanth.ddit@gmail.com>
@sriv
Copy link
Contributor Author

sriv commented Sep 23, 2020

The build is presently failing in golang <= 1.9, because strings.Builder is not available. I see that travis builds are setup for go 1.3+

Just wanted to check if the project aims to be compatible with go <=1.9, in which case I can use plain string concatenation.

properties.go Outdated Show resolved Hide resolved
@magiconair
Copy link
Owner

go1.3 compatibility isn't strictly necessary but I've been enjoying the fact that this library is usable with all the runtimes over the last 6 years that it has been somewhat of a pet peeve to keep it that way. If it is really necessary to break it then I'll do it. However, I think you can just use bytes.Buffer and it'll work.

@sriv
Copy link
Contributor Author

sriv commented Sep 23, 2020

Fair enough, and I think it makes sense especially when the change is as simple as you suggested. Should have thought of it myself, thanks for the quick review.

@magiconair magiconair merged commit e6c7fa7 into magiconair:master Sep 23, 2020
@magiconair
Copy link
Owner

Hmm, so while being fun it isn't very helpful...

image

@magiconair
Copy link
Owner

10 min for a CI run isn't really acceptable

@magiconair
Copy link
Owner

Tagged v1.8.4. Thank you!

@magiconair magiconair added this to the 1.8.4 milestone Feb 23, 2022
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.

None yet

2 participants