Add more position conversion functions #1223
Conversation
rocky
changed the title
| @@ -1,28 +1,60 @@ | |||
| import { isAstNode, AstWalker } from './astWalker'; | |||
| import { AstNode, Location } from "./types"; | |||
| import { AstNode, LineColPosition, LineColRange, Location } from "./types"; | |||
| const util = require("remix-lib").util; | |||
There was a problem hiding this comment.
could we use here the import ... from ?
There was a problem hiding this comment.
Sure. Note though this was not as easy as just changing that line...
remix-lib is not in typescript so there are not types for that and I could not find a @types/remix-lib. Rather than undertake writing such a module, I opted for starting one of the one function I need in remix-astWalker. I am new to this stuff, so I don't know if this was done properly.
| * @returns {LineColPosition} | ||
| */ | ||
| export function lineColPositionFromOffset(offset: number, lineBreaks: Array<number>): LineColPosition { | ||
| let line: number = util.findLowerBound(offset, lineBreaks); |
There was a problem hiding this comment.
we are basically moving this from
, which seems needed. But we can't keep both code, we should deprecate the one inremix-lib
There was a problem hiding this comment.
I am assuming this is just a comment. If there was some action I should take here, let me know.
There was a problem hiding this comment.
I think once remix-astWalker gets a bit mature, we can start removing existing codes we are copying/migrating from.
There was a problem hiding this comment.
Would love to see that and will help to make sure this is integrated to your atom plugin. See also #1224, #1225, #1226, #1227 which @yann300 asked me to log as issues.
Also although I haven't logged as an issue, the whole walker function could probably be async which might improve performance. I haven't logged that as an issue though.
#1230 is not really related but it is a big annoyance in vscode because it brings in keccak which vscode barfs on badly. (I need to isolate what's up there and report to Microsoft. I recall it has been reported in the past, but not isolated.)
There was a problem hiding this comment.
Also I should say that I am sure there are lots of opportunities for making things efficient. But first we should have the basic functionality working, and right now in the plain walk routine I don't think that is the case. (This is a little bit an artifact of the current solc AST design which I believe will be documented better in the future.)
There was a problem hiding this comment.
I think once remix-astWalker gets a bit mature, we can start removing existing codes we are copying/migrating from.
sounds good
- type LineColPosition, - type LineColRange, - function lineColPositionFromOffset - function srcToLineColumnRange And associated tests for these
rocky
changed the title
And associated tests for these.
To discuss:
One thing I would like to see is clarification of things in
remix-astWalker/src/types.tslikeLocation.Here it means the interpretation of a solc-style AST-field
src. I think then that should be made explicit likesolcLocation.Although I personally like the solcLocation for its compactness, more common is a line/column based range. And here there are a couple of variations depending on whether columns and lines start at 0 or 1.
Since I am using this stuff in a LSP oriented way, I have adopted their convention of both lines and columns starting at 1. Personally I am okay with adding more kinds of things for other ways to represent a location.