-
-
Notifications
You must be signed in to change notification settings - Fork 356
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
cmd/gosh: refactor into a multi-purpose tool #330
Comments
Since we're working on v3, I'd likely also remove |
Concerning /**
* ArrayElem represents a Bash array element.
*/
type ArrayElemGeneric<Base, Pos, Op> = Base & {
type: 'ArrayElem';
/** [i]=, ["k"]= */
Index: null | ArithmExprGeneric<Base, Pos, Op>;
Value: WordGeneric<Base, Pos, Op>;
Comments: CommentGeneric<Base, Pos, Op>[];
} They should agree with the output of
|
The only clarification I'd make here is that I'd squash anonymous fields. For example,
We can't have funcs in JSON, so I agree. I'm not sure about the JS package though. Would be nice if the methods reflected any changes made to the fields. But methods might be too complex to do if we do the translation from Go to generic JS/JSON objects ourselves.
Agreed, as far as nil-able fields go. That's all pointers and interfaces.
Agreed. I went with
I was thinking of leaving operators as strings, actually. In Go, they're numbers simply because that's the simplest way to describe enums. Operators are essentially strings, so the string would need to be in the JSON somewhere.
If you mean missing positions like a statement's semicolon position, sure. I left them out of |
Sure. I do too when I translate your trees into my interpreter's format.
Forgot to mention I leave them as an optional property i.e.
I am happy to use
I got it wrong. I translate them to
I'd prefer them to have a |
Are those methods/functions? If so, are they implemented in terms of the methods on the JS objects that the current JS package gives you?
I honestly don't mind. Using Someone on Hacker News suggested a while ago that I should replace all of my
I think there's an important distinction to make here. There are lots of lexer tokens, some of which are then surfaced as operators in the syntax tree. The only reason why the operator enum values are a subset of the token enums is for simplicity; conversion back and forth is just a type conversion. So the JS package should only expose operators, not tokens. If we encounter any invalid value like
You mean always, or just in the "short" mode? |
Yes, they refer to the original functions. The typings I'm talking about don't exist at runtime. They are intended to describe what is in your javascript package, and are very helpful during development. They differ slightly because I mutate your data structures slightly: adding interface Pos {
type: 'Pos';
/**
* After reports whether this position p is after p2. It is a more expressive version of p.Offset() > p2.Offset().
*/
After(p2: Pos): boolean;
/**
* Col returns the column number of the position, starting at 1. It counts in bytes.
*/
Col(): number;
/**
* IsValid reports whether the position is valid. All positions in nodes returned by Parse are valid.
*/
IsValid(): boolean;
/**
* Line returns the line number of the position, starting at 1.
*/
Line(): number;
/**
* Offset returns the byte offset of the position in the original source file. Byte offsets start at 0.
*/
Offset(): number;
String(): string;
} They are also parametric in
Agreed. The typings can't change the names anyway, they are merely a reflection.
This problem arises because I need to convert an instance of an integer-valued enum into a string. Inasmuch as this conversion is needed, I use a line-by-line translation of your enum into an array: export class ParseShService {
private readonly opMetas: {
name: string;
value: null | string;
}[] = [
{ name: 'illegalTok', value: null },
{ name: '_EOF', value: null },
{ name: '_Newl', value: null },
{ name: '_Lit', value: null },
{ name: '_LitWord', value: null },
{ name: '_LitRedir', value: null },
{ name: 'sglQuote', value: '\'' },
{ name: 'dblQuote', value: '"' },
{ name: 'bckQuote', value: '`' },
/** etc. */ I have to use Alternatively you could provide a utility function in your original package, exposing it in your js package. Even if GopherJS provided
My preference would be to always explicitly include |
I've decided not to do this. Reasons:
The ideas that I think are still worth doing, like |
shfmt
has grown in popularity, since it's a useful tool with a unique name. However, its purpose is also narrow - it simply formats shell programs.This is a pity, because this repository holds plenty of features beyond parsing and printing shell code. I propose that
gosh
be turned into a shell "swiss army knife", with features such as:gosh expand
- like https://godoc.org/mvdan.cc/sh/shell#Expandgosh source
- like https://godoc.org/mvdan.cc/sh/shell#SourceFilegosh quote
- to quote shell words, like syntax: add a Quote API #328gosh format
- an identical copy ofshfmt
gosh find
- to find all shell scripts within a directory, like the currentshfmt -f
gosh parse
- to parse a shell file and give the syntax tree in JSON, like the currentshfmt -tojson
gosh print
- to take a syntax tree in JSON and print it, the reverse ofparse
There would still be a place for
shfmt
, of course. It's well established, much simpler and smaller, and essentially a different tool. If I could go back in time, perhaps I'd have published it as justgosh format
, but that's out of the question now.The current
gosh
shell could be renamed togosh-shell
, since it's mostly meant as a debugging and experimental tool. We can't have that be onlygosh shell
, as the only way to configure the system shell is to use a path to a binary.The text was updated successfully, but these errors were encountered: