-
Notifications
You must be signed in to change notification settings - Fork 24
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
Generic doc cleanup #2938
Generic doc cleanup #2938
Conversation
|
The latest updates on your projects. Learn more about Vercel for Git ↗︎
1 Ignored Deployment
|
Apply Sweep Rules to your PR?
|
Fixing PR: track the progress here.I'm currently fixing this PR to address the following: [Sweep GHA Fix] The GitHub Actions run failed with the following error logs:
An error has occurred: None (tracking ID: 32994af75b) |
* main: Added changelog Tag.all -> Tag.getAll Deprecate Plot titles reimplement updateSquiggleLangVersion with babel move update-system-version to ops scripts reorganize release scripts validate package.json with zod
* main: (25 commits) fix 404 bug in relative values fix react warnings remove @quri/configs from changeset-ignored packages remove broken TextTooltip from settings icon support imports in relative values models update module=esnext in tsconfig constants.ts for NEXT_PUBLIC vars bump module for nextjs apps to es2022 more lint fixes Use REThrow for Common.throw() lint fix ViewerProvider is a singleton now; wrap SquiggleOutputViewer top level in it update changeset config common tsconfigs Hot fix for Tag.hide Fixes Prettier bug where single-item dict key trailing comma would be removed Delete sweep.yaml CR comments Adds Common.throw() Added 'Common.throw' fn ...
* main: Responded to CRs Bring variableName to sidebar ModelEvaluator should take a variableName, instead of only working for 'fn' Minor tuneups to Relative Value plots Fixed basic plots for Relative Value display
* main: skip 0.9.1 release, prepare for 0.9.2 textmate fix changeset remove 0.9.1 from versioned-components fix packaged files don't rebuild squiggle-lang, System.version is now updated through ops/ Bump versions after 0.9.1 release version for @quri/configs (see pnpm/pnpm#4164 (comment)) update website changelog, new generate-website-changelog script manually update changelogs Version Packages
Codecov ReportAll modified and coverable lines are covered by tests ✅
Additional details and impacted files@@ Coverage Diff @@
## main #2938 +/- ##
==========================================
+ Coverage 69.73% 69.78% +0.04%
==========================================
Files 120 120
Lines 6791 6788 -3
Branches 1448 1448
==========================================
+ Hits 4736 4737 +1
+ Misses 2047 2043 -4
Partials 8 8 ☔ View full report in Codecov by Sentry. |
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 didn't read all docs changes but what I've read looks good, happy to see more descriptions.
@@ -12,14 +12,27 @@ import { | |||
|
|||
type Shorthand = { type: "infix" | "unary"; symbol: string }; | |||
|
|||
type example = { |
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.
type example = { | |
type Example = { |
@@ -28,6 +41,10 @@ export type FRFunction = { | |||
versionAdded?: string; | |||
}; | |||
|
|||
function organizedExamples(f: FRFunction) { | |||
return [...(f.examples ?? [])]; |
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.
Minor coding style note, so feel free to ignore, but this looks excessive to me, both the separate function and the way you copy the entire array (you could return f.examples ?? []
)`.
To expand on our old "inline or not inline" argument again:
- if you skipped the function, it would enforce on those who need
examples
something like(f.examples ?? [])
orf.examples?.map(...) ?? []
, which doesn't look that bad to me; TypeScript will stop you from making a mistake there - I don't expect this function to need extra logic ever
- if I'm wrong about this and the assumption is that we will need to preprocess examples eventually (filter out deprecated stuff or whatever), then I'd argue that exposing
examples
onFRFunction
is wrong anyway, because it's easy to forget this call, andFRFunction
should be a class with a getter or a method
})`, | ||
{ isInteractive: true } | ||
), | ||
makeFnExample( |
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.
Instead of adding makeFnExample
wrapper everywhere, I'd propose auto-wrapping plain string examples
values.
We have so many stdlib functions that it's worth it to make the syntax for the most common case as terse as possible.
I expect you'll prefer to postpone this idea since you already did the conversion, but here's how I'd do it:
- patch
SimplifiedArgs
inregistry/helpers.ts
, omitexamples
and addexamples: (string | Example)[]
- then map
typeof example === 'string' ? makeFnExample(example) : example
inFnFactory.make
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.
Sigh... good point. I did it when I wasn't feeling good anyway, so it was an easy-ish non-intense project to do. Will see about changing later on.
@@ -11,4 +13,18 @@ Interactive visualizations are a primary tool for understanding Squiggle code, b | |||
- **Variable Settings Toggle:** Click on the variable menu in the Squiggle interface and select "Log to JS Console". | |||
|
|||
## ``Window.squiggleOutput`` |
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.
## ``Window.squiggleOutput`` | |
## ``window.squiggleOutput`` |
Capital-W Window
exists but it's a class name.
@@ -22,7 +22,15 @@ export const modulePages: ModulePage[] = [ | |||
{ | |||
description: "Dates are a simple date time type.", | |||
name: "Date", | |||
intro: ``, | |||
intro: `A simple date type. Dates are stored as milliseconds since the epoch. They are immutable, and all functions that modify dates return a new date. Used with [Duration](./Duration) values. |
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.
They are immutable
Maybe "like any other value in Squiggle, they are immutable"? Otherwise, depending on the order in which I read documentation, I'd end up confused for a while.
</div> | ||
) : ( | ||
<MarkdownViewer | ||
className="max-width-[200px]" |
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.
max-width-[200px]
This is not new, but (1) it's broken (tailwind syntax is max-w-[...]
, not max-width-[...]
), (2) doesn't seem useful anyway?
Merging, will fix some of these later, thank you! |
No description provided.