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

sparse-v2: design doc proposition for Sparse Patterns refactoring #2877

Merged
merged 1 commit into from
Feb 2, 2024

Conversation

torquestomp
Copy link
Contributor

Proposal for a Sparse Patterns rewrite, to address multiple feature gaps.
Looking for feedback and approval before moving forward with real changes.

Checklist

If applicable:

  • I have updated CHANGELOG.md
  • I have updated the documentation (README.md, docs/, demos/)
  • I have updated the config schema (cli/src/config-schema.json)
  • I have added tests to cover my changes

@torquestomp torquestomp force-pushed the dploch/sparse-patterns-v2 branch from 220e5eb to dfed901 Compare January 23, 2024 21:26
Copy link
Contributor

@PhilipMetzger PhilipMetzger left a comment

Choose a reason for hiding this comment

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

Here are some initial comments. I'd also prefer if you structured it like one of the existing Design Docs, maybe the style of tracking-branches fits for this document. Oh and don't forget to add it to mkdocs.yml or it won't be in the design section of the website.

docs/design/sparse-v2.md Show resolved Hide resolved
docs/design/sparse-v2.md Outdated Show resolved Hide resolved
docs/design/sparse-v2.md Outdated Show resolved Hide resolved
docs/design/sparse-v2.md Outdated Show resolved Hide resolved
docs/design/sparse-v2.md Outdated Show resolved Hide resolved
docs/design/sparse-v2.md Outdated Show resolved Hide resolved
@torquestomp torquestomp force-pushed the dploch/sparse-patterns-v2 branch 6 times, most recently from 125841e to 1e62edb Compare January 25, 2024 21:51
Copy link
Contributor

@PhilipMetzger PhilipMetzger left a comment

Choose a reason for hiding this comment

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

I actually really like the design from the first revision on, it just really lacks context for a broader (non-Google3) audience.

docs/design/sparse-v2.md Outdated Show resolved Hide resolved
docs/design/sparse-v2.md Outdated Show resolved Hide resolved
docs/design/sparse-v2.md Outdated Show resolved Hide resolved
docs/design/sparse-v2.md Outdated Show resolved Hide resolved
docs/design/sparse-v2.md Outdated Show resolved Hide resolved
docs/design/sparse-v2.md Outdated Show resolved Hide resolved
docs/design/sparse-v2.md Outdated Show resolved Hide resolved
docs/design/sparse-v2.md Outdated Show resolved Hide resolved
docs/design/sparse-v2.md Outdated Show resolved Hide resolved
docs/design/sparse-v2.md Outdated Show resolved Hide resolved
@arxanas
Copy link
Contributor

arxanas commented Jan 28, 2024

Maybe you could compare to Subversion's "externals" or https://github.com/josh-project/josh?

docs/design/sparse-v2.md Outdated Show resolved Hide resolved
docs/design/sparse-v2.md Outdated Show resolved Hide resolved
docs/design/sparse-v2.md Outdated Show resolved Hide resolved
docs/design/sparse-v2.md Outdated Show resolved Hide resolved
docs/design/sparse-v2.md Outdated Show resolved Hide resolved
docs/design/sparse-v2.md Outdated Show resolved Hide resolved
docs/design/sparse-v2.md Outdated Show resolved Hide resolved
docs/design/sparse-v2.md Outdated Show resolved Hide resolved
docs/design/sparse-v2.md Outdated Show resolved Hide resolved
docs/design/sparse-v2.md Outdated Show resolved Hide resolved
@torquestomp torquestomp force-pushed the dploch/sparse-patterns-v2 branch from 1e62edb to 1ae7176 Compare January 31, 2024 20:10
Copy link
Contributor

@PhilipMetzger PhilipMetzger left a comment

Choose a reason for hiding this comment

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

LGTM, it's probably just missing a related work section. I'm going to leave the rest of the discussion in concerns to internal details to the respective stakeholders.

@torquestomp
Copy link
Contributor Author

LGTM, it's probably just missing a related work section. I'm going to leave the rest of the discussion in concerns to internal details to the respective stakeholders.

Thanks for the review! Sorry for harping on any previously-discussed points; there was a lot to read through.
I'll do some touch-ups tomorrow before pinging the other convos.

@torquestomp
Copy link
Contributor Author

Maybe you could compare to Subversion's "externals" or https://github.com/josh-project/josh?

Added the josh project to "related work"; I don't think Subversion's "externals" are related here.

@torquestomp
Copy link
Contributor Author

LGTM, it's probably just missing a related work section. I'm going to leave the rest of the discussion in concerns to internal details to the respective stakeholders.

Added a "Related Work" section.

@torquestomp torquestomp force-pushed the dploch/sparse-patterns-v2 branch from 1ae7176 to a77433e Compare February 1, 2024 23:54
@torquestomp torquestomp enabled auto-merge (rebase) February 1, 2024 23:55
@torquestomp torquestomp merged commit 0182595 into main Feb 2, 2024
15 checks passed
@torquestomp torquestomp deleted the dploch/sparse-patterns-v2 branch February 2, 2024 00:02
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

5 participants