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

(feat) add repo metadata to export csv result#50857

Merged
erzhtor merged 3 commits into
mainfrom
erzhtor/add-repo-metadata-to-export-csv-file
Apr 21, 2023
Merged

(feat) add repo metadata to export csv result#50857
erzhtor merged 3 commits into
mainfrom
erzhtor/add-repo-metadata-to-export-csv-file

Conversation

@erzhtor

@erzhtor erzhtor commented Apr 19, 2023 •
edited
Loading

Copy link
Copy Markdown
Contributor

Part of https://github.com/sourcegraph/pr-faqs/issues/96.

Test plan

  • sg start
  • Enable the repository-metadata feature flag
  • Add some key-value pairs through API (currently no UI for adding) GQL mutation addRepoKeyValuePair API
  • Search with select:repo and export results

Screenshot

image

@erzhtor erzhtor requested review from a team April 19, 2023 13:08
@erzhtor erzhtor self-assigned this Apr 19, 2023
@cla-bot cla-bot Bot added the cla-signed label Apr 19, 2023
@sourcegraph-bot

sourcegraph-bot commented Apr 19, 2023 •
edited
Loading

Copy link
Copy Markdown
Contributor

Codenotify: Notifying subscribers in CODENOTIFY files for diff 3cadca9...f09abde.

Notify File(s)
@fkling client/shared/src/search/stream.ts
client/web/src/search/results/export/SearchResultsCsvExportModal.tsx
client/web/src/search/results/export/searchResultsExport.test.ts
client/web/src/search/results/export/searchResultsExport.ts
@limitedmage client/web/src/search/results/export/SearchResultsCsvExportModal.tsx
client/web/src/search/results/export/searchResultsExport.test.ts
client/web/src/search/results/export/searchResultsExport.ts

@sg-e2e-regression-test-bob

sg-e2e-regression-test-bob commented Apr 19, 2023 •
edited
Loading

Copy link
Copy Markdown

Bundle size report 📦

Initial size Total size Async size Modules
0.00% (0.00 kb) 0.00% (+0.25 kb) 0.00% (+0.25 kb) 0.00% (0)

Look at the Statoscope report for a full comparison between the commits f09abde and 3cadca9 or learn more.

Open explanation
  • Initial size is the size of the initial bundle (the one that is loaded when you open the page)
  • Total size is the size of the initial bundle + all the async loaded chunks
  • Async size is the size of all the async loaded chunks
  • Modules is the number of modules in the initial bundle

limitedmage
limitedmage reviewed Apr 19, 2023
View reviewed changes
Comment thread client/web/src/search/results/export/searchResultsExport.ts Outdated
new URL(getRepositoryUrl(result.repository, result.branches), sourcegraphURL).toString(),
]),
]
content = !enableRepositoryMetadata

@limitedmage limitedmage Apr 19, 2023

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.

A lot of repeated code in these two branches of the ternary operation. Can we instead modify the function inside the map to return the right value based on the condition?

@erzhtor erzhtor Apr 20, 2023

Copy link
Copy Markdown
Contributor Author

Choose a reason for hiding this comment

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

Thank you for the suggestion—updated f09abde.

P.S: Also, by the end of the prfaq, all these feature flag checks/conditions will be removed since the feature be enabled for GA, which will make the code more straightforward.

@erzhtor
refactor(searchResultsExport.ts): simplify searchResultsToFileContent…
f09abde 
… function by removing duplicate code and using ternary operator to conditionally add repository metadata headers and values
vdavid
vdavid approved these changes Apr 20, 2023
View reviewed changes

@vdavid vdavid 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.

LGTM!

Not sure how those unrelated Percy changes got to this branch 😕 they have nothing to do with this PR, but they might block you form merging this hmm. I'd reach out to devX of #dev-frontend about them.

@erzhtor

erzhtor commented Apr 20, 2023

Copy link
Copy Markdown
Contributor Author

LGTM!

Not sure how those unrelated Percy changes got to this branch 😕 they have nothing to do with this PR, but they might block you form merging this hmm. I'd reach out to devX of #dev-frontend about them.

Thanks for the review, @vdavid. Regarding Percy snapshots, yes they are not related to the PR, but I assume they are flaky ones. As far as I know percy job is not required, but rather a good way to understand what changes were introduced whether intentionally or accidentally.

@erzhtor erzhtor merged commit e018d96 into main Apr 21, 2023
@erzhtor erzhtor deleted the erzhtor/add-repo-metadata-to-export-csv-file branch April 21, 2023 07:58
Sign up for free to subscribe to this conversation on GitHub. Already have an account? Sign in.

Reviewers

@camdencheek camdencheek Awaiting requested review from camdencheek

2 more reviewers

@limitedmage limitedmage limitedmage approved these changes

@vdavid vdavid vdavid approved these changes

Reviewers whose approvals may not affect merge requirements

Assignees

@erzhtor erzhtor

Labels

cla-signed

Projects

None yet

Milestone

No milestone

Development

Successfully merging this pull request may close these issues.

5 participants

@erzhtor @sourcegraph-bot @sg-e2e-regression-test-bob @limitedmage @vdavid