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.

Sign up for GitHub

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

Jump to bottom

[refactor] Type-driven refactoring: remove span_id in PrefixPrint() #1491

Merged
merged 1 commit into from
Feb 7, 2023
Merged

[refactor] Type-driven refactoring: remove span_id in PrefixPrint() #1491

Show file tree
Hide file tree
Changes from all commits
Commits
Show all changes
1 commit
Select commit
File filter

Filter by extension

Filter by extension
Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
2 changes: 1 addition & 1 deletion core/executor.py
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -139,7 +139,7 @@ def RunBuiltin(self, builtin_id, cmd_val):
if e.span_id == runtime.NO_SPID:
e.span_id = self.errfmt.CurrentLocation()
# e.g. 'type' doesn't accept flag '-x'
self.errfmt.PrefixPrint(e.msg, prefix='%r ' % arg0, span_id=e.span_id)
self.errfmt.PrefixPrint(e.msg, '%r ' % arg0, loc.Span(e.span_id))
Copy link
Contributor Author

Choose a reason for hiding this comment

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

This is the common case: simply wrap span ID in loc.Span()

status = 2 # consistent error code for usage error

return status
Expand Down
7 changes: 4 additions & 3 deletions core/ui.py
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -11,7 +11,7 @@

from _devbuild.gen.id_kind_asdl import Id, Id_t, Id_str
from _devbuild.gen.syntax_asdl import (
Token, SourceLine, command_t, command_str,
loc_t, Token, SourceLine, command_t, command_str,
source_e, source__Stdin, source__MainFile, source__SourcedFile,
source__Alias, source__Reparsed, source__Variable, source__VarRef,
source__ArgvWord, source__Synthetic
Expand Down Expand Up @@ -304,9 +304,10 @@ def CurrentLocation(self):
else:
return runtime.NO_SPID

def PrefixPrint(self, msg, prefix, span_id=runtime.NO_SPID):
# type: (str, str, int) -> None
def PrefixPrint(self, msg, prefix, blame_loc):
# type: (str, str, loc_t) -> None
"""Print a hard-coded message with a prefix, and quote code."""
span_id = location.GetSpanId(blame_loc)
Copy link
Contributor Author

Choose a reason for hiding this comment

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

we can use our function to convert a loc_t to a span_id

Later will remove it from this internal function, but for now it lets us make a small diff

_PrintWithSpanId(prefix, msg, span_id, self.arena, show_code=True)

def Print_(self, msg, span_id=runtime.NO_SPID):
Expand Down
4 changes: 2 additions & 2 deletions osh/cmd_eval.py
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -316,7 +316,7 @@ def _RunAssignBuiltin(self, cmd_val):
arg0 = cmd_val.argv[0]
if e.span_id == runtime.NO_SPID: # fill in default location.
e.span_id = self.errfmt.CurrentLocation()
self.errfmt.PrefixPrint(e.msg, prefix='%r ' % arg0, span_id=e.span_id)
self.errfmt.PrefixPrint(e.msg, '%r ' % arg0, loc.Span(e.span_id))
status = 2 # consistent error code for usage error

return status
Expand Down Expand Up @@ -1046,7 +1046,7 @@ def _Dispatch(self, node, cmd_st):
else:
# Only print warnings, never fatal.
# Bash oddly only exits 1 for 'return', but no other shell does.
self.errfmt.PrefixPrint(msg, prefix='warning: ', span_id=tok.span_id)
self.errfmt.PrefixPrint(msg, 'warning: ', tok)
Copy link
Contributor Author

@andychu andychu Feb 6, 2023 •
edited
Loading

Choose a reason for hiding this comment

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

Here we do NOT pass loc.Span() -- we just pass the token itself -- because we happen to have it available!

Remember that a Token is a location

Anywhere you would see loc.Span(tok.span_id) -- that's wrong and should just be tok

That's like doing a "double conversion" and is not necessary

status = 0

# Note CommandList and DoGroup have no redirects, but BraceGroup does.
Expand Down
2 changes: 0 additions & 2 deletions soil/worker.sh
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -447,8 +447,6 @@ job-main() {

log-context 'job-main'
mkdir -v -p $out_dir

set -x
ls -l -d $out_dir

disable-git-errors
Expand Down