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(rust_indexer): Fix proxy indexer #5032

Merged
merged 4 commits into from
Aug 11, 2021
Merged

Conversation

wcalandro
Copy link
Contributor

No description provided.

@google-cla google-cla bot added the cla: yes label Aug 11, 2021
Copy link
Contributor

@jaysachs jaysachs left a comment

Choose a reason for hiding this comment

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

I sorta wish this was 3 separate PRs:

  1. hex -> base64 dependency
  2. removal of path from FileProvider methods
  3. the actual fix ("true" -> true)

@shahms
Copy link
Contributor

shahms commented Aug 11, 2021

Why remove path when it exists on the underlying RPC?

@wcalandro
Copy link
Contributor Author

I sorta wish this was 3 separate PRs:

  1. hex -> base64 dependency
  2. removal of path from FileProvider methods
  3. the actual fix ("true" -> true)

I agree that the FileProvider change could probably be broken out, but the "true" -> true change and the hex -> base64 change are both part of the fix

@wcalandro
Copy link
Contributor Author

Why remove path when it exists on the underlying RPC?

Based on looking at other indexers that utilize the proxy interface, the precedent is to completely ignore the path argument.

@shahms
Copy link
Contributor

shahms commented Aug 11, 2021

Why remove path when it exists on the underlying RPC?

Based on looking at other indexers that utilize the proxy interface, the precedent is to completely ignore the path argument.

Right, but it exists to allow for implementations to ignore either and provide better diagnostics in either case.

@wcalandro wcalandro merged commit d1ccce0 into kythe:master Aug 11, 2021
@wcalandro wcalandro deleted the fix-proxy branch April 4, 2022 17:54
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
Projects
None yet
Development

Successfully merging this pull request may close these issues.

None yet

3 participants