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

scripts: add todo_diff.sh #70871

Merged
merged 1 commit into from
Sep 29, 2021
Merged

Conversation

ajwerner
Copy link
Contributor

Adds a shell script to find TODOs that have been added between two commits.

Relates to #62552.

Release note: None

@ajwerner ajwerner requested a review from mgartner September 29, 2021 16:17
@cockroach-teamcity
Copy link
Member

This change is Reviewable

@ajwerner
Copy link
Contributor Author

$ ./scripts/todo_diff.sh v21.1.8 release-21.2  > todo_diff.txt
Submodule path 'c-deps/proj': checked out '9016763fb90519740ce1be1a1956505ca0710f7d'
Submodule path 'pkg/ui/yarn-vendor': checked out '52c9d5d7fe3766f6f9b6cf3400d3e84b84112691'
Submodule path 'vendor': checked out 'aed63b0be4b1995d5797bca5963ac404f60b2fdc'
warning: unable to rmdir 'c-deps/protobuf': Directory not empty
Submodule path 'c-deps/proj': checked out 'c8ff95857beb3027b5aa3d15726795570f38eccb'
Submodule path 'pkg/ui/yarn-vendor': checked out '0454bb3216cbc5847f7147df2d07e33cd04aad78'
Submodule path 'vendor': checked out 'e677f187b87c5e48118ac663e806b6fab4f6ab45'

generates:
todo_diff.txt

Copy link
Collaborator

@mgartner mgartner left a comment

Choose a reason for hiding this comment

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

Looks great!

It would be nice if we can filter out lines like:

pkg/sql/catalog/schemadesc/synthetic_schema_desc.go: log.Fatalf(context.TODO(),

Copy link
Collaborator

@rytaft rytaft left a comment

Choose a reason for hiding this comment

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

Thanks for doing this!

Reviewable status: :shipit: complete! 0 of 0 LGTMs obtained (waiting on @ajwerner and @rytaft)


scripts/todo_diff.sh, line 33 at r1 (raw file):

  
  # Checkout the new files for the sql directory.
  git checkout --quiet "${UPSTREAM}/${BRANCH}" pkg/sql

Consider allowing the user to pass in the directory of interest instead of hard coding pkg/sql


scripts/todo_diff.sh, line 38 at r1 (raw file):

  git reset --quiet HEAD pkg
  
  # Add all untracked files to the index so the will show results with git diff.

nit: so the will -> so they will

@ajwerner ajwerner force-pushed the ajwerner/add-todo_diff.sh branch 2 times, most recently from c0a835e to 42c2204 Compare September 29, 2021 17:10
Copy link
Contributor Author

@ajwerner ajwerner left a comment

Choose a reason for hiding this comment

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

It would be nice if we can filter out lines like:
pkg/sql/catalog/schemadesc/synthetic_schema_desc.go: log.Fatalf(context.TODO()

Done. Added commentary around the grep.

Reviewable status: :shipit: complete! 0 of 0 LGTMs obtained (waiting on @rytaft)


scripts/todo_diff.sh, line 33 at r1 (raw file):

Previously, rytaft (Rebecca Taft) wrote…

Consider allowing the user to pass in the directory of interest instead of hard coding pkg/sql

Good catch, the mismatch between pkg/sql and pkg below was a bug.

./scripts/todo_diff.sh v21.1.8 release-21.2 pkg/sql/opt now gives you:

pkg/sql/opt/cat/column.go: // TODO(janexing): implement support for OVERRIDING SYSTEM VALUE
pkg/sql/opt/exec/execbuilder/testdata/inverted_index: # TODO(angelazxu): The {} span does not need to be scanned here, but is
pkg/sql/opt/exec/execbuilder/testdata/scan_parallel: # TODO(yuzefovich): use DISTSQL option - for some reason at the moment we
pkg/sql/opt/exec/execbuilder/testdata/scan_parallel: # TODO(yuzefovich): use DISTSQL option - for some reason at the moment we
pkg/sql/opt/exec/execbuilder/testdata/union: # TODO(yuzefovich): The synchronizers in the below DistSQL plans are all
pkg/sql/opt/invertedidx/json_array.go: // TODO(mgartner): The first argument could be an expression that matches a
pkg/sql/opt/memo/logical_props_builder.go: // TODO(rytaft): Support other limited types such as INT2, BIT(N), VARBIT(N),
pkg/sql/opt/memo/logical_props_builder.go: // TODO(rytaft): This should really be an assertion failure. See #67434.
pkg/sql/opt/memo/statistics_builder.go: // TODO(mgartner): It might be simpler to iterate over all the table columns
pkg/sql/opt/memo/statistics_builder.go: // TODO(mgartner): Calculate and set the column statistics based on
pkg/sql/opt/optbuilder/mutation_builder.go: // TODO(janexing): to implement the OVERRIDING SYSTEM VALUE syntax
pkg/sql/opt/optbuilder/scope_column.go: // TODO(mgartner): Do we still need this?
pkg/sql/opt/ordering/interesting_orderings.go: // TODO(radu): perhaps take into account right-side interesting orderings on
pkg/sql/opt/ordering/set.go: // TODO(rytaft): Consider changing the execution engine to accept the
pkg/sql/opt/props/ordering_choice.go: // TODO(justin): investigate picking an order more intelligently here.
pkg/sql/opt/xform/coster.go: // TODO(treilly): do some empirical analysis and model this better
pkg/sql/opt/xform/general_funcs.go: // TODO(drewk): handle inverted scans.
pkg/sql/opt/xform/join_funcs.go: // TODO(mgartner): We could handle this by wrapping the lookup join in
pkg/sql/opt/xform/physical_props.go: // TODO(mgartner): If the expression is a streaming DistinctOn, this
pkg/sql/opt/xform/placeholder_fast_path.go: // TODO(radu): if we want to support more cases, we should use optgen to write
pkg/sql/opt/xform/rules/groupby.opt: # TODO(rytaft): add support for ensure-distinct-on and upsert-distinct-on
pkg/sql/opt/xform/rules/groupby.opt: # TODO(rytaft): add support for ensure-distinct-on and upsert-distinct-on
pkg/sql/opt/xform/scan_funcs.go: // TODO(rytaft): Revisit this when we have a more accurate cost model for data
pkg/sql/opt/xform/testdata/external/tpce: # TODO: The limit could be pushed down if limit push-down rules could see
pkg/sql/opt/xform/testdata/external/tpce: # TODO: The limit should be pushed down, but the projects (added during
pkg/sql/opt/xform/testdata/external/tpce-no-stats: # TODO: The limit could be pushed down if limit push-down rules could see
pkg/sql/opt/xform/testdata/external/tpce-no-stats: # TODO: The limit should be pushed down, but the projects (added during
pkg/sql/opt/xform/testdata/physprops/ordering: # TODO(rytaft): We could remove the ordering from the INTERSECT operation
pkg/sql/opt/xform/testdata/rules/join: # TODO(mgartner): The right side of the join can "produce" columns held constant
pkg/sql/opt/xform/testdata/rules/join: # TODO(mgartner): We could handle this by wrapping the lookup join in an
pkg/sql/opt/xform/testdata/rules/join: # TODO(mgartner): We could handle this by wrapping the lookup join in an
pkg/sql/opt/xform/testdata/rules/join: # TODO(mgartner): We could handle this by wrapping the lookup join in an index
pkg/sql/opt/xform/testdata/rules/join: # TODO(mgartner): We could handle this by wrapping the lookup join in an index
pkg/sql/opt/xform/testdata/rules/select: # TODO(mgartner): Once again, we find ourselves with a suboptimal query plan

scripts/todo_diff.sh, line 38 at r1 (raw file):

Previously, rytaft (Rebecca Taft) wrote…

nit: so the will -> so they will

Done.

@ajwerner ajwerner force-pushed the ajwerner/add-todo_diff.sh branch from 42c2204 to 54a56e9 Compare September 29, 2021 17:55
Copy link
Collaborator

@mgartner mgartner left a comment

Choose a reason for hiding this comment

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

💯

Copy link
Collaborator

@rytaft rytaft left a comment

Choose a reason for hiding this comment

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

:lgtm:

Reviewed 1 of 1 files at r4, all commit messages.
Reviewable status: :shipit: complete! 1 of 0 LGTMs obtained (waiting on @ajwerner)

@rytaft
Copy link
Collaborator

rytaft commented Sep 29, 2021


scripts/todo_diff.sh, line 39 at r4 (raw file):

  git checkout --quiet "tags/${TAG}"

  # Checkout the new files for the sql directory.

nit: sql -> search

Adds a shell script to find `TODO`s that have been added between two commits.

Relates to cockroachdb#62552.

Release note: None
@ajwerner ajwerner force-pushed the ajwerner/add-todo_diff.sh branch from 54a56e9 to d992049 Compare September 29, 2021 20:38
Copy link
Contributor Author

@ajwerner ajwerner left a comment

Choose a reason for hiding this comment

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

TFTR!

bors r+

Reviewable status: :shipit: complete! 0 of 0 LGTMs obtained (and 1 stale) (waiting on @rytaft)


scripts/todo_diff.sh, line 39 at r4 (raw file):

Previously, rytaft (Rebecca Taft) wrote…

nit: sql -> search

Done.

@craig
Copy link
Contributor

craig bot commented Sep 29, 2021

Build succeeded:

@craig craig bot merged commit 03d3160 into cockroachdb:master Sep 29, 2021
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.

4 participants