Skip to content
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

Merged
merged 14 commits into from
Jan 14, 2024
Merged

Generic doc cleanup #2938

merged 14 commits into from
Jan 14, 2024

Conversation

OAGr
Copy link
Contributor

@OAGr OAGr commented Jan 9, 2024

No description provided.

Copy link

changeset-bot bot commented Jan 9, 2024

⚠️ No Changeset found

Latest commit: bd31fea

Merging this PR will not cause a version bump for any packages. If these changes should not result in a new version, you're good to go. If these changes should result in a version bump, you need to add a changeset.

This PR includes no changesets

When changesets are added to this PR, you'll see the packages that this PR includes changesets for and the associated semver types

Click here to learn what changesets are, and how to add one.

Click here if you're a maintainer who wants to add a changeset to this PR

Copy link

vercel bot commented Jan 9, 2024

The latest updates on your projects. Learn more about Vercel for Git ↗︎

Name Status Preview Updated (UTC)
quri-hub ✅ Ready (Inspect) Visit Preview Jan 13, 2024 4:58am
squiggle-components ✅ Ready (Inspect) Visit Preview Jan 13, 2024 4:58am
squiggle-website ✅ Ready (Inspect) Visit Preview Jan 13, 2024 4:58am
1 Ignored Deployment
Name Status Preview Updated (UTC)
quri-ui ⬜️ Ignored (Inspect) Visit Preview Jan 13, 2024 4:58am

Copy link
Contributor

sweep-ai bot commented Jan 9, 2024

Apply Sweep Rules to your PR?

  • Apply: All docstrings and comments should be up to date.
  • Apply: Ensure that all variables and functions have descriptive names.
  • Apply: Avoid using magic numbers or hard-coded values in the code.

Copy link
Contributor

sweep-ai bot commented Jan 9, 2024

Sweeping

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:



[!CAUTION]

An error has occurred: None (tracking ID: 32994af75b)

Copy link
Contributor

sweep-ai bot commented Jan 9, 2024

Sweeping

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:



Created Pull Request: #2939

OAGr added 2 commits January 9, 2024 11:55
* 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
  ...
OAGr added 3 commits January 10, 2024 19:50
* 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
Copy link

codecov bot commented Jan 12, 2024

Codecov Report

All modified and coverable lines are covered by tests ✅

Comparison is base (5be13ee) 69.73% compared to head (d23462e) 69.78%.
Report is 7 commits behind head on main.

❗ Current head d23462e differs from pull request most recent head bd31fea. Consider uploading reports for the commit bd31fea to get more accurate results

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.
📢 Have feedback on the report? Share it here.

Copy link
Collaborator

@berekuk berekuk left a 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 = {
Copy link
Collaborator

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Suggested change
type example = {
type Example = {

@@ -28,6 +41,10 @@ export type FRFunction = {
versionAdded?: string;
};

function organizedExamples(f: FRFunction) {
return [...(f.examples ?? [])];
Copy link
Collaborator

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 ?? []) or f.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 on FRFunction is wrong anyway, because it's easy to forget this call, and FRFunction should be a class with a getter or a method

})`,
{ isInteractive: true }
),
makeFnExample(
Copy link
Collaborator

@berekuk berekuk Jan 13, 2024

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 in registry/helpers.ts, omit examples and add examples: (string | Example)[]
  • then map typeof example === 'string' ? makeFnExample(example) : example in FnFactory.make

Copy link
Contributor Author

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``
Copy link
Collaborator

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Suggested change
## ``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.
Copy link
Collaborator

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]"
Copy link
Collaborator

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?

@OAGr OAGr merged commit cfbbdca into main Jan 14, 2024
6 checks passed
@OAGr OAGr deleted the Doc-refactors-Jan-8 branch January 14, 2024 01:07
@OAGr
Copy link
Contributor Author

OAGr commented Jan 14, 2024

Merging, will fix some of these later, thank you!

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
Archived in project
Development

Successfully merging this pull request may close these issues.

2 participants