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

add generativeDirectAnswer support #170

Merged
merged 10 commits into from
Jun 12, 2024
Merged

add generativeDirectAnswer support #170

merged 10 commits into from
Jun 12, 2024

Conversation

anguyen-yext2
Copy link

J=CLIP-1226
TEST=auto,manual

added and ran unit test
tested in test-site-node, saw appropriate gda response

EmilyZhang777 and others added 3 commits January 26, 2024 10:06
J=CLIP-1226
TEST=auto,manual

added and ran unit test
tested in test-site-node, saw appropriate gda response
J=CLIP-1226
TEST=auto,manual

added and ran unit test
tested in test-site-node, saw appropriate gda response
@anguyen-yext2 anguyen-yext2 requested a review from a team as a code owner June 6, 2024 16:59
@coveralls
Copy link

coveralls commented Jun 6, 2024

Pull Request Test Coverage Report for Build 9404901272

Details

  • 9 of 15 (60.0%) changed or added relevant lines in 1 file are covered.
  • No unchanged relevant lines lost coverage.
  • Overall coverage decreased (-1.5%) to 91.429%

Changes Missing Coverage Covered Lines Changed/Added Lines %
src/search-headless.ts 9 15 60.0%
Totals Coverage Status
Change from base Build 7669778155: -1.5%
Covered Lines: 347
Relevant Lines: 372

💛 - Coveralls

@coveralls
Copy link

coveralls commented Jun 6, 2024

Pull Request Test Coverage Report for Build 9404910688

Details

  • 9 of 15 (60.0%) changed or added relevant lines in 1 file are covered.
  • No unchanged relevant lines lost coverage.
  • Overall coverage decreased (-1.5%) to 91.429%

Changes Missing Coverage Covered Lines Changed/Added Lines %
src/search-headless.ts 9 15 60.0%
Totals Coverage Status
Change from base Build 7669778155: -1.5%
Covered Lines: 347
Relevant Lines: 372

💛 - Coveralls

@coveralls
Copy link

coveralls commented Jun 6, 2024

Pull Request Test Coverage Report for Build 9404918882

Details

  • 9 of 15 (60.0%) changed or added relevant lines in 1 file are covered.
  • No unchanged relevant lines lost coverage.
  • Overall coverage decreased (-1.5%) to 91.429%

Changes Missing Coverage Covered Lines Changed/Added Lines %
src/search-headless.ts 9 15 60.0%
Totals Coverage Status
Change from base Build 7669778155: -1.5%
Covered Lines: 347
Relevant Lines: 372

💛 - Coveralls

Copy link
Collaborator

@yen-tt yen-tt left a comment

Choose a reason for hiding this comment

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

didn't note this in search-core, but since this is beta and still in revision process, you may want to merge this into a separate branch instead of main to avoid blocking any (also typically you would merge to develop branch. Update to main is only for when it's time for publishing an official version). You may need to update search-core's develop branch as well (see this PR)

Here would be my suggestion:

  • make a fork branch (e.g. gda-beta) from develop
  • push all your changes to that branch and publish betas from there
  • once iterations is done and we are ready to publish to the official version, merge gda-beta to develop. and develop to master when release cycle come
    This would be the flow for all the SDK repos, to avoid blocking hotfixes / official feature updates.

cc @EmilyZhang777 for additional input

src/search-headless.ts Outdated Show resolved Hide resolved
src/search-headless.ts Show resolved Hide resolved
etc/search-headless.api.md Outdated Show resolved Hide resolved
src/search-headless.ts Outdated Show resolved Hide resolved
@EmilyZhang777
Copy link
Contributor

didn't note this in search-core, but since this is beta and still in revision process, you may want to merge this into a separate branch instead of main to avoid blocking any (also typically you would merge to develop branch. Update to main is only for when it's time for publishing an official version). You may need to update search-core's develop branch as well (see this PR)

Here would be my suggestion:

  • make a fork branch (e.g. gda-beta) from develop
  • push all your changes to that branch and publish betas from there
  • once iterations is done and we are ready to publish to the official version, merge gda-beta to develop. and develop to master when release cycle come
    This would be the flow for all the SDK repos, to avoid blocking hotfixes / official feature updates.

cc @EmilyZhang777 for additional input

Sorry that I missed this on the original PR.
There's a auto generated PR to merge master into dev. Can we just use that to get the change into develop and then revert the original PR?

@coveralls
Copy link

coveralls commented Jun 10, 2024

Pull Request Test Coverage Report for Build 9455766796

Details

  • 29 of 36 (80.56%) changed or added relevant lines in 2 files are covered.
  • No unchanged relevant lines lost coverage.
  • Overall coverage decreased (-1.6%) to 91.367%

Changes Missing Coverage Covered Lines Changed/Added Lines %
src/search-headless.ts 24 31 77.42%
Totals Coverage Status
Change from base Build 7669778155: -1.6%
Covered Lines: 367
Relevant Lines: 393

💛 - Coveralls

Copy link
Collaborator

@yen-tt yen-tt left a comment

Choose a reason for hiding this comment

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

please update this PR to merge to develop instead

etc/search-headless.api.md Outdated Show resolved Hide resolved
src/search-headless.ts Outdated Show resolved Hide resolved
src/search-headless.ts Outdated Show resolved Hide resolved
src/search-headless.ts Outdated Show resolved Hide resolved
@vijay267 vijay267 requested a review from a team June 11, 2024 16:09
@coveralls
Copy link

coveralls commented Jun 11, 2024

Pull Request Test Coverage Report for Build 9470476962

Details

  • 27 of 34 (79.41%) changed or added relevant lines in 2 files are covered.
  • No unchanged relevant lines lost coverage.
  • Overall coverage decreased (-1.7%) to 91.241%

Changes Missing Coverage Covered Lines Changed/Added Lines %
src/search-headless.ts 22 29 75.86%
Totals Coverage Status
Change from base Build 7669778155: -1.7%
Covered Lines: 365
Relevant Lines: 391

💛 - Coveralls

@anguyen-yext2 anguyen-yext2 changed the base branch from main to develop June 11, 2024 17:58
Copy link
Contributor

@EmilyZhang777 EmilyZhang777 left a comment

Choose a reason for hiding this comment

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

overall LGTM! Just two nits

src/search-headless.ts Outdated Show resolved Hide resolved
src/search-headless.ts Outdated Show resolved Hide resolved
@coveralls
Copy link

coveralls commented Jun 11, 2024

Pull Request Test Coverage Report for Build 9472519838

Details

  • 27 of 34 (79.41%) changed or added relevant lines in 2 files are covered.
  • No unchanged relevant lines lost coverage.
  • Overall coverage decreased (-1.7%) to 91.241%

Changes Missing Coverage Covered Lines Changed/Added Lines %
src/search-headless.ts 22 29 75.86%
Totals Coverage Status
Change from base Build 8469147869: -1.7%
Covered Lines: 365
Relevant Lines: 391

💛 - Coveralls

@coveralls
Copy link

coveralls commented Jun 11, 2024

Pull Request Test Coverage Report for Build 9472688747

Details

  • 27 of 34 (79.41%) changed or added relevant lines in 2 files are covered.
  • No unchanged relevant lines lost coverage.
  • Overall coverage decreased (-1.7%) to 91.241%

Changes Missing Coverage Covered Lines Changed/Added Lines %
src/search-headless.ts 22 29 75.86%
Totals Coverage Status
Change from base Build 8469147869: -1.7%
Covered Lines: 365
Relevant Lines: 391

💛 - Coveralls

@anguyen-yext2 anguyen-yext2 merged commit 40c8918 into develop Jun 12, 2024
8 checks passed
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