Skip to content
This repository was archived by the owner on Sep 30, 2024. It is now read-only.

notebooks: set default pattern type to keyword#63662

Merged
stefanhengl merged 2 commits into
mainfrom
sh/kw/notebooks-new-default
Jul 9, 2024
Merged

notebooks: set default pattern type to keyword#63662
stefanhengl merged 2 commits into
mainfrom
sh/kw/notebooks-new-default

Conversation

@stefanhengl

@stefanhengl stefanhengl commented Jul 5, 2024

Copy link
Copy Markdown
Member

Relates to https://github.com/sourcegraph/sourcegraph/pull/63472

This changes the default patternType of Notebooks from "standard" to "keyword". As a consequence, all new notebooks will default to keyword search. Existing notebooks will keep using standard search.

Test plan:

  • I ran the migration locally and verified that new notebooks use "keyword" as default.

Relates to https://github.com/sourcegraph/sourcegraph/pull/63472

This changes the default patternType of Notebooks from "standard" to
"keyword".

The migration introduced in
https://github.com/sourcegraph/sourcegraph/pull/63472 sets all
*existing* Notebooks to pattern type "standard". The migration of this PR
will cause all *new* Notebooks to use "keyword".

Test plan:
I ran the migration locally a verified that new notebooks use "keyword"
as default.
@cla-bot cla-bot Bot added the cla-signed label Jul 5, 2024
@github-actions github-actions Bot added team/product-platform team/search-platform Issues owned by the search platform team labels Jul 5, 2024
@stefanhengl stefanhengl requested a review from a team July 5, 2024 11:04
@stefanhengl stefanhengl marked this pull request as ready for review July 5, 2024 11:05

@jtibshirani jtibshirani left a comment

Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

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

Makes sense! Just musing: I prefer to handle default logic at the application layer, so you don't need a database migration to change it. I feel it's more obvious to newcomers too (we don't usually think to look at the schema layer for important application logic).

I see we already use postgres SET DEFAULT in other places, and my opinion may be the minority, so no need to change anything for this PR!

@stefanhengl

Copy link
Copy Markdown
Member Author

Makes sense! Just musing: I prefer to handle default logic at the application layer, so you don't need a database migration to change it. I feel it's more obvious to newcomers too (we don't usually think to look at the schema layer for important application logic).

I see we already use postgres SET DEFAULT in other places, and my opinion may be the minority, so no need to change anything for this PR!

I see what you mean and you are right, we could do it all on the application layer. However, the approach of storing the default pattern type in the db was a hack to keep the overall code changes to a minimum.

Additionally, in this case, I think having a default enforced by the db is nice, because it guarantees a valid "state" of the notebook. The client relies on the backend to always send the pattern type. We could build this guarantee into the application layer of course but it seems much cleaner to do it with the db. And, maybe the most important point. This is something we don't change very often, and in the case of Notebooks, probably never (famous last words ;-)).

@stefanhengl stefanhengl merged commit e3d112b into main Jul 9, 2024
@stefanhengl stefanhengl deleted the sh/kw/notebooks-new-default branch July 9, 2024 07:36
Sign up for free to subscribe to this conversation on GitHub. Already have an account? Sign in.

Labels

cla-signed team/product-platform team/search-platform Issues owned by the search platform team

Projects

None yet

Development

Successfully merging this pull request may close these issues.

2 participants