notebooks: store default pattern type per notebook#63472
Conversation
Based on your Notion doc, I thought we were not introducing V4 in this first "Phase 1" of work. And that instead, we would persist |
For Notebooks, it is not as straightforward to add the pattern type to the query as it is for Code Monitors or Insights. The blocks are multiline, support comments, have autosave, and are collaborative. IMO it is the best experience if the default pattern type is defined per Notebook and not per block. Now the question is whether we persist the default pattern type or the version in the db. Functionally, both are the same. I am slightly in favour of using "version" here because it fits to our long term vision to require a "version" whenever we accept or persist a query. As for the rollout: For "Phase 1", I don't think we should introduce "version" to the API, but we can add support for "V4" to the code without breaking anything. |
It feels slightly messy to me to introduce V4 before it's available in the API. We also want "Phase 1" to be self contained and not introduce any tech debt. Plus, Notebooks might be deprecated soon, so it's not a big deal if the schema includes pattern type directly. What do you think? By the way, this PR looks great other than this question. |
I swapped version with pattern type. PTAL @jtibshirani |
jtibshirani
left a comment
There was a problem hiding this comment.
This looks good to me! Thanks for the iterations. I just had one question around the allowed pattern types.
| export function fetchStreamSuggestionsPatternTypeVersion( | ||
| query: string, | ||
| patternType: SearchPatternType, | ||
| version: string, |
There was a problem hiding this comment.
Do we need to expose version yet? Maybe we're just doing this as part of our "be a good citizen and pass version explicitly" efforts?
There was a problem hiding this comment.
This doesn't really expose "version". The APIs already expose "version" and it is already part of StreamSearchOptions. This change merely makes this option available to the helper fetchStreamSuggestions. However, it's not critical for this PR, so we can punt on it.
| 'literal', | ||
| 'regexp', | ||
| 'standard', | ||
| 'structural' |
There was a problem hiding this comment.
Would it make sense to only include keyword, regexp, and standard? Can you actually default to structural search in notebooks?
There was a problem hiding this comment.
The default is determined by what we define as default for notebooks.field pattern_type, so "no", user are not able to choose the default by themselves. That being said, notebooks do support structural search.
The db type pattern_type is not limited to notebooks, that's why I included all non-experimental pattern types.
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.
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.
This PR refactors notebooks to support the upcoming Keyword Search GA. The main goal is to make it easier to switch to a new default pattern type without breaking existing notebooks.
Before
After
notebooks.pattern_type(defaults to "standard").Notes
patternTypeandversionare both required parameters, which is redundant, because version acts as a default pattern type already. I left a TODO in the code for that. I don't want to change this as part of this PR because the change would get very big and affect too much code outside of Notebooks..snb.mdextension. Unlike notebooks stored in our db, we cannot migrate those files.Q&A
Q: How does this help for Keyword Search GA?
A: Once we default to keyword search, we can change the default of
notebooks.pattern_typefrom "standard" to "keyword". Existing notebooks will still work with "standard". New Notebooks will use "keyword".Q: How can customers migrate existing notebooks to a new version?
A: Use the existing "Copy to My Notebooks" function of Notebooks. The copied notebook will have the current default pattern type.
Test plan:
notebooks.pattern_typeto make sure that the request parameters change.