-
Notifications
You must be signed in to change notification settings - Fork 14
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: fixed condition parsing and display #37
Conversation
Would be helpful if you could add before after screenshots. |
No UI changes |
0b57a01
to
519da2c
Compare
let dimension_name = operands | ||
.iter() | ||
.find_map(|item| match item.as_object() { | ||
Some(o) if o.contains_key("var") => { |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
can we have better name instead of "o"
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
hey, o
is just a throw away variable not even being used anywhere instead of that Some()
arm, if its affecting readability I will change, if not lets go with this.
In my opinion I don't really think we need a full word for variable name for these kind of scenarios.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
you are using o in o.contains_key
and in o["var"]
, when you are operating on any variable it would be better if it is not just o,a,b,c,d single alphabet,
atleast val
would work
Other(String), | ||
} | ||
|
||
impl Display for ConditionOperator { |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Can we use strum here ?
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
There was no way in strum to serialize this Other(String)
into to just use the inner value, so went ahead with this approach.
519da2c
to
b9a2c2a
Compare
b9a2c2a
to
5472c54
Compare
5472c54
to
b7eee77
Compare
Problem
We had inconsistent parsing logic for context, which led to wrong data display of context.
Solution
Fix the parsing logic