-
Notifications
You must be signed in to change notification settings - Fork 20
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
Remove remnants from "LSIF Typed" #191
Conversation
The codebase had a lot of outdated references to "LSIF Typed", which has been renamed to SCIP. This commit replaces these outdated references to avoid confusion for people who are reading the codebase for the first time.
cc/ @mrnugget @valerybugakov who both asked about the usage of LSIF in scip-typescript |
type Descriptor = scip.scip.Descriptor | ||
const Descriptor = scip.scip.Descriptor | ||
type Suffix = scip.scip.Descriptor.Suffix | ||
const Suffix = scip.scip.Descriptor.Suffix |
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.
nit: Can we somehow avoid this being scip.scip
. It's not a big deal, but it looks a little gross lol.
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 agree it's not nice and I'd +1 if we can solve it somehow. It's not a regression from this PR though, it was gross before as well
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 still copy-paste scip.ts from the sourcegraph/scip repo. It would be nice to publish the bindings to npm so that we can add a normal package.json dependency instead. We can do that later, however, this PR is already a big improvement.
Any idea why the CI is not triggering? |
import { Packages } from './Packages' | ||
import { Range } from './Range' | ||
import * as scip from './scip' | ||
import { ScipSymbol } from './ScipSymbol' |
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.
nit: Following JS/TS conventions, I think this should be called SCIPSymbol, similar to HTML and URL in browser APIs.
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 recall seeing in relation to XMLHttpRequest
that the convention is that only three-letter acronym should remain uppercase. I really dislike SCIPSymbol, it's easier IMO to read ScipSymbol so I prefer to keep it
Nothing seems to have changed in the CI config, so I'm as confused as you are. Some transient GitHub issue? Maybe try pushing a dummy commit? |
Empty commit seems to have done the trick |
The codebase had a lot of outdated references to "LSIF Typed", which has been renamed to SCIP. This commit replaces these outdated references to avoid confusion for people who are reading the codebase for the first time.
Test plan
See the CI go green. This should be a pure refactoring, no functional difference.