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: inconsistent evaluation of tm_try() #432

Merged
merged 5 commits into from
Jun 29, 2022

Conversation

katcipis
Copy link
Contributor

No description provided.

@katcipis katcipis requested a review from i4ki June 28, 2022 16:39
@katcipis katcipis self-assigned this Jun 28, 2022
@codecov
Copy link

codecov bot commented Jun 28, 2022

Codecov Report

Merging #432 (851877d) into main (2dfccb3) will decrease coverage by 0.03%.
The diff coverage is 100.00%.

@@            Coverage Diff             @@
##             main     #432      +/-   ##
==========================================
- Coverage   69.70%   69.67%   -0.04%     
==========================================
  Files          42       42              
  Lines        7745     7736       -9     
==========================================
- Hits         5399     5390       -9     
  Misses       2094     2094              
  Partials      252      252              
Flag Coverage Δ
tests 69.67% <100.00%> (-0.04%) ⬇️

Flags with carried forward coverage won't be shown. Click here to find out more.

Impacted Files Coverage Δ
stack/globals.go 95.90% <100.00%> (-0.21%) ⬇️

Continue to review full report at Codecov.

Legend - Click here to learn more
Δ = absolute <relative> (impact), ø = not affected, ? = missing data
Powered by Codecov. Last update 2dfccb3...851877d. Read the comment docs.

@katcipis katcipis marked this pull request as ready for review June 28, 2022 16:54
Comment on lines +668 to +672
want: map[string]*hclwrite.Block{
"/stack": globals(
str("a", "value"),
str("b", "value"),
str("c", "value"),
Copy link
Contributor

Choose a reason for hiding this comment

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

❤️

{
path: "/stack",
add: globals(
str("a_interpolated", "${global.undefined}-something"),
Copy link
Contributor

Choose a reason for hiding this comment

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

👍

Copy link
Contributor

@mariux mariux left a comment

Choose a reason for hiding this comment

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

lgtm

this should not be a breaking change afaiu?

it's a change in behavior but stuff that worked before is still the same result?

Copy link
Contributor

@i4ki i4ki left a comment

Choose a reason for hiding this comment

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

🚀

@@ -111,18 +111,15 @@ func (ge *globalsExpr) eval(rootdir string, meta Metadata) (Globals, error) {
// This is relative only to root since meta.Path will look
// like: /some/path/relative/project/root
logger := log.With().
Str("action", "eval()").
Str("action", "globals.eval()").
Copy link
Contributor

Choose a reason for hiding this comment

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

Nice!

The action metadata is currently very inconsistent.

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Yeah it is, I tried to check if the logger had some easy way to get method/function names but I didn't find anything at the time (wanted to remove actions in general), only a way to always log filename + line etc. The problem with using things like just "eval" is that we have a bunch of eval related stuff..so it can get confusing to understand the logs, would be cool to be consistent in a way to provide more context on the actions.

@katcipis
Copy link
Contributor Author

lgtm

this should not be a breaking change afaiu?

it's a change in behavior but stuff that worked before is still the same result?

It should not be a breaking change, the only change in behavior is that code that used to fail before will now work, if you are pedantic it could be considered a breaking change, because maybe some people depended on the breaking/failing behavior, but I don't think we need to be that pedantic at this point =P

@katcipis katcipis merged commit ac2b598 into main Jun 29, 2022
@katcipis katcipis deleted the katcipis-fix-try-with-undefined-reference branch June 29, 2022 10:01
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

3 participants