-
-
Notifications
You must be signed in to change notification settings - Fork 50
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
refactor: migrate arkdark to arktype repo #755
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.
Looks good overall, a couple tweaks. Going to take a look at it locally and see if anything jumps out, but hopefully will be fine.
README.md
Outdated
@@ -19,7 +19,7 @@ | |||
<!-- @snipEnd --> | |||
|
|||
```ts @blockFrom:dev/test/examples/type.ts | |||
import { type } from "arktype" | |||
import { type } from "../../../src/main.js" |
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.
This is a regression but I think it's just because I manually changed them recently 😂 Can you either manually change them back or fix the snip transform so they import from arktype
?
dev/arkdark/.vscode/launch.json
Outdated
@@ -0,0 +1,13 @@ | |||
// A launch configuration that launches the extension inside a new window |
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.
Could we move this to the repo root and still have it open from ./dev/arkdark
?
dev/arkdark/.vsixmanifest
Outdated
@@ -0,0 +1,45 @@ | |||
<?xml version="1.0" encoding="utf-8"?> |
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.
Where does this keep coming from 🤣
dev/arkdark/LICENSE.txt
Outdated
@@ -0,0 +1,21 @@ | |||
MIT License | |||
|
|||
Copyright (c) 2021 re-do |
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.
What's "re-do"?
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.
(you can just delete all this stuff it is covered by the repo root LICENSE)
dev/arkdark/arktype.test.ts
Outdated
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.
Let's rename this to arkdark.scratch.ts
so it doesn't look like we're supposed to be running it as a test.
dev/arkdark/package.json
Outdated
} | ||
] | ||
}, | ||
"__metadata": { |
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.
Is this all supposed to be here? I don't think I've ever seen it before.
dev/arkdark/tsconfig.json
Outdated
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.
Can we get rid of this and rely on the root tsconfig? If that doesn't work it's fine to keep it
dev/arkdark/themes/arkDark.json
Outdated
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.
This needs to be .gitignored
, or change the name to arkDark.temp.json
or something so it will match one of our existing ignores.
if (this.snapRequiresUpdate(expectedSerialized)) { | ||
const snapshotArgs: SnapshotArgs = { | ||
position: caller(), | ||
serializedValue: this.serializedActual | ||
position: caller({ |
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.
Maybe we revert these now that they're not needed. People will be confused by the nonexistent hack 😅
@@ -68,7 +68,7 @@ export const formatFilePath = ( | |||
|
|||
export const caller = (options: CallerOfOptions = {}): SourcePosition => { | |||
let upStackBy = options.upStackBy ?? 0 | |||
if (!options.methodName) { | |||
if (!options.methodName && !options.upStackBy) { |
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.
This we keep, definitely an improvement 👍
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.
Looks great. Excited to try the new highlighting, looks much better 💯
@@ -10,6 +10,7 @@ tmp | |||
.cache-loader | |||
.attest | |||
coverage | |||
/**/themes/*.json |
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.
Ideally in the future this should be the specific path to the folder you want to exclude, since it's not like this is being generated anywhere else
Move Arkdark into Dev
Fixed bleeding highlights