-
Notifications
You must be signed in to change notification settings - Fork 72
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
chore: only warn about KeyError "ast"/"metadata" if debugging #331
Conversation
+ a drive-by simplification of Mapper
src/halmos/__main__.py
Outdated
except KeyError as err: | ||
if str(err) in ["ast", "metadata"]: | ||
# somewhat expected, e.g. for sol files containing only libraries | ||
if args.debug: | ||
debug( | ||
f"Skipped {json_filename} due to parsing failure: " |
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.
this is the real change, the rest is a refactoring
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.
what's your intention when err is not ast nor metadata? generate warning, or silently ignore?
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.
another q: would it be possible to check if the ast / metadata missing error is indeed due to libraries?
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.
another q: would it be possible to check if the ast / metadata missing error is indeed due to libraries?
that was the case every time I looked
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.
what's your intention when err is not ast nor metadata? generate warning, or silently ignore?
oops, the intention is to generate a warning for other errors
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.
what's your intention when err is not ast nor metadata? generate warning, or silently ignore?
oops, the intention is to generate a warning for other errors
in that case, i don't think the current code works as intended.
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.
fixed in b135251
except Exception: | ||
pass |
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.
this is what prompted the refactoring, we don't want to ignore all exceptions
@@ -1300,8 +1296,24 @@ def mk_arrlen(args: HalmosConfig) -> Dict[str, int]: | |||
return arrlen | |||
|
|||
|
|||
def parse_symbols(args: HalmosConfig, contract_map, contract_name): |
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.
pulling this in its own method to avoid nested try..except blocks
def parse_build_out(args: HalmosConfig) -> Dict: | ||
result = {} # compiler version -> source filename -> contract name -> (json, type) | ||
# compiler version -> source filename -> contract name -> (json, type) | ||
result = defaultdict(lambda: defaultdict(dict)) |
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.
opportunistic, this avoids doing if x not in y: y[x] = {}
a bunch of times
if not json_filename.endswith(".json"): | ||
continue | ||
if json_filename.startswith("."): | ||
continue | ||
|
||
json_path = os.path.join(sol_path, json_filename) | ||
with open(json_path, encoding="utf8") as f: | ||
json_out = json.load(f) | ||
json_path = os.path.join(sol_path, json_filename) | ||
with open(json_path, encoding="utf8") as f: | ||
json_out = json.load(f) |
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.
pulling this out of the try..except block to reduce the scope of the try blocks and minimize nesting
should address #281 |
could you separate the main change from the refactoring changes, to make it easier to review the core change? could be a separate pr, or just a separate commit. |
[starting a new thread for easier discussion]
ah, my question was, whether it's possible to automatically check if the problematic file is a library or not, and generate the error message only when it is not a library. indeed, i'm concerned about silently ignoring field-missing errors in non-debug mode. although we haven't seen cases where non-library files are skipped due to missing fields, i believe we still need to allow users review the skipped files, unless we're certain they can be safely skipped. also, the usability issue described in #281 is not about the warning message itself, but rather that the message is not detailed enough to understand what's happening, and its tone is too strong. so, my suggestion for the short term is to improve the warning message, allowing users to review them and detect any real issues, without being scared. a longer-term solution is to implement a mechanism to check whether the given file is a library and ignore the error only when it's a library. |
closing this one, it's getting too messy. Will update the investigation in #281 |
plus a drive-by simplification of Mapper