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鈥檒l occasionally send you account related emails.

Already on GitHub? Sign in to your account

feat: return text when content type is text/csv #579

Merged
merged 3 commits into from Jan 30, 2024

Conversation

vinteo
Copy link
Contributor

@vinteo vinteo commented Oct 13, 2023

Thank you for opening a Pull Request! Before submitting your PR, there are a few things you can do to make sure it goes smoothly:

  • Make sure to open an issue as a bug/issue before writing your code! That way we can discuss the change, evaluate designs, and agree on the general idea
  • Ensure the tests and linter pass
  • Code coverage does not decrease (if any source code was changed)
  • Appropriate docs were updated (if necessary)

Fixes #578 馃

@vinteo vinteo requested a review from a team as a code owner October 13, 2023 01:48
@product-auto-label product-auto-label bot added the size: s Pull request size is small. label Oct 13, 2023
src/gaxios.ts Outdated
@@ -404,6 +404,7 @@ export class Gaxios {
return data as {};
} else if (
contentType.includes('text/plain') ||
contentType.includes('text/csv') ||
Copy link
Contributor

Choose a reason for hiding this comment

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

Now I'm just thinking if we should instead do contentType.startsWith('text/') and consider all text content the same. @sofisl what do you think?

Copy link
Contributor

Choose a reason for hiding this comment

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

I agree! Would you mind including a regex instead? and maybe a .match? Thank you!

Copy link
Contributor

@alexander-fenster alexander-fenster left a comment

Choose a reason for hiding this comment

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

Thank you! We'll merge it and release soon.

@alexander-fenster alexander-fenster added the kokoro:run Add this label to force Kokoro to re-run the tests. label Oct 13, 2023
@yoshi-kokoro yoshi-kokoro removed the kokoro:run Add this label to force Kokoro to re-run the tests. label Oct 13, 2023
src/gaxios.ts Outdated
@@ -404,6 +404,7 @@ export class Gaxios {
return data as {};
} else if (
contentType.includes('text/plain') ||
contentType.includes('text/csv') ||
Copy link
Contributor

Choose a reason for hiding this comment

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

I agree! Would you mind including a regex instead? and maybe a .match? Thank you!

contentType.includes('text/plain') ||
contentType.includes('text/html')
) {
} else if (contentType.match(/^text\//)) {
Copy link
Contributor Author

Choose a reason for hiding this comment

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

not sure how specific the regular expression should be, hope this suits.

@danielbankhead danielbankhead changed the title fix: return text when content type is text/csv feat: return text when content type is text/csv Dec 6, 2023
@vinteo
Copy link
Contributor Author

vinteo commented Jan 16, 2024

Any chance of pushing this through?

@danielbankhead danielbankhead added kokoro:force-run Add this label to force Kokoro to re-run the tests. owlbot:run Add this label to trigger the Owlbot post processor. labels Jan 30, 2024
@gcf-owl-bot gcf-owl-bot bot removed the owlbot:run Add this label to trigger the Owlbot post processor. label Jan 30, 2024
@yoshi-kokoro yoshi-kokoro removed the kokoro:force-run Add this label to force Kokoro to re-run the tests. label Jan 30, 2024
@danielbankhead danielbankhead merged commit 3cc1c76 into googleapis:main Jan 30, 2024
15 checks passed
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
size: s Pull request size is small.
Projects
None yet
Development

Successfully merging this pull request may close these issues.

Using googleapis for direct CSV downloads does not return CSV
5 participants