-
Notifications
You must be signed in to change notification settings - Fork 457
Factor source code-related facilities into a new package #269
Conversation
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.
LGTM
@@ -0,0 +1,21 @@ | |||
MIT License | |||
|
|||
Copyright (c) 2015-2019 GitHub |
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.
A lot of this code has been sitting around since 2015. (At a minimum, the Range
stuff.)
Source.Loc | ||
Source.Range | ||
Source.Source | ||
Source.Span |
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.
IMO we should format .cabal
files this way in general. Aligning crap to the right of labels is terrible.
import Source.Range | ||
import Source.Span | ||
|
||
data Loc = Loc |
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.
I opted to abbreviate Location
to Loc
because we use it everywhere.
|
||
data Loc = Loc | ||
{ byteRange :: {-# UNPACK #-} !Range | ||
, span :: {-# UNPACK #-} !Span |
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.
Furthermore, and in keeping with our style guide, I’ve opted to rename its fields to drop the prefix.
|
||
|
||
byteRange_ :: Lens' Loc Range | ||
byteRange_ = lens byteRange (\l r -> l { byteRange = r }) |
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.
I also gave it a handy lens to modify its range. We could do a classy-lenses–style HasRange
eventually but for now this is it.
|
||
|
||
instance CustomHasPackageDef Language.Go.Syntax.Package where | ||
customToPackageDef Blob{..} _ (Language.Go.Syntax.Package (Term (In fromAnn _), _) _) | ||
= Just $ PackageDef (getSource fromAnn) | ||
where getSource = toText . flip Source.slice blobSource . locationByteRange | ||
where getSource = toText . Source.slice blobSource . byteRange |
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.
We flipped Source.slice
’s definition since we so frequently had to flip its uses instead.
@@ -180,7 +180,7 @@ define :: ( HasCallStack | |||
-> Evaluator term address value m () | |||
define declaration rel accessControl def = withCurrentCallStack callStack $ do | |||
-- TODO: This span is still wrong. | |||
declare declaration rel accessControl emptySpan Unknown Nothing | |||
declare declaration rel accessControl lowerBound Unknown Nothing |
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.
emptySpan
is gone; we rely on the Lower
instance instead.
span = lens infoSpan (\i s -> i { infoSpan = s }) | ||
{-# INLINE span #-} | ||
span_ = lens infoSpan (\i s -> i { infoSpan = s }) | ||
{-# INLINE span_ #-} |
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.
We renamed the Span
fields to start
and end
, and now follow the convention that lens names end in _
.
toJSONFields sourceSpan = [ "sourceSpan" .= sourceSpan ] | ||
|
||
instance ToJSONFields Loc where | ||
toJSONFields Loc{..} = toJSONFields byteRange <> toJSONFields span |
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.
All these instances live here now so we didn’t have to move ToJSONFields
into semantic-source
or anything.
|
||
sourceLineRangesByLineNumber :: Source -> Array Int Range | ||
sourceLineRangesByLineNumber source = listArray (1, length lineRanges) lineRanges | ||
where lineRanges = Source.lineRanges source |
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.
These were only ever being used in this module, and so moving them here prevented having to have semantic-source
depend on array
.
One last bit: we’ve opted to leave |
This PR factors
Source
,Range
,Span
,Location
, &c. out into a new package,semantic-source
, intended to be used bysemantic
,semantic-ast
,semantic-tags
,semantic-core
,semantic-python
, andhaskell-tree-sitter
.