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

Fix: permit saving structural searches #17265

Merged
merged 1 commit into from Jan 25, 2021
Merged

Conversation

jensim
Copy link
Contributor

@jensim jensim commented Jan 14, 2021

It is currently not possible to save a structural search due to this

var patternTypeRegexp = lazyregexp.New(`(?i)\bpatternType:(literal|regexp|structural)\b`)

It is currently not possible to save a structural search due to this 
```
var patternTypeRegexp = lazyregexp.New(`(?i)\bpatternType:(literal|regexp|structural)\b`)
```
@rvantonder
Copy link
Contributor

Hi @jensim, thanks for this! I'm going to double check that this is the only change we need to get structural saves searches to work. Tracking issue: #7316

@rvantonder rvantonder self-requested a review January 15, 2021 17:54
@jensim
Copy link
Contributor Author

jensim commented Jan 16, 2021

Hi @jensim, thanks for this! I'm going to double check that this is the only change we need to get structural saves searches to work. Tracking issue: #7316

I patched my OSS instance with it and was able to save and load a structural search, but I guess e2e tests might be in order?

@codecov
Copy link

codecov bot commented Jan 18, 2021

Codecov Report

Merging #17265 (3aaa4ed) into main (21b1b82) will decrease coverage by 0.00%.
The diff coverage is n/a.

@@            Coverage Diff             @@
##             main   #17265      +/-   ##
==========================================
- Coverage   52.10%   52.10%   -0.01%     
==========================================
  Files        1711     1711              
  Lines       85337    85337              
  Branches     7629     7629              
==========================================
- Hits        44465    44461       -4     
- Misses      36954    36959       +5     
+ Partials     3918     3917       -1     
Flag Coverage Δ *Carryforward flag
go 51.19% <ø> (-0.01%) ⬇️
integration 30.54% <ø> (ø) Carriedforward from 21b1b82
storybook 30.10% <ø> (ø) Carriedforward from 21b1b82
typescript 54.31% <ø> (ø) Carriedforward from 21b1b82
unit 34.83% <ø> (ø) Carriedforward from 21b1b82

*This pull request uses carry forward flags. Click here to find out more.

Impacted Files Coverage Δ
cmd/frontend/graphqlbackend/saved_searches.go 32.25% <ø> (ø)
.../internal/codeintel/resolvers/graphql/locations.go 83.50% <0.00%> (-2.07%) ⬇️
...nal/campaigns/resolvers/changeset_apply_preview.go 58.97% <0.00%> (-1.71%) ⬇️

@rvantonder
Copy link
Contributor

Hey @jensim this looks good--I pushed the changes to our CI and the build is green. I want to add an entry to our CHANGELOG but I couldn't edit this PR. I think this can work by checking a box, but it's easier for me to just approve this and get it merged, and will add the CHANGELOG separately.

@rvantonder rvantonder merged commit 30eb219 into sourcegraph:main Jan 25, 2021
@rvantonder
Copy link
Contributor

Thanks again!

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.

None yet

2 participants