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

feat(datasource/custom): add toml support #28594

Open
wants to merge 2 commits into
base: main
Choose a base branch
from

Conversation

imkarrer
Copy link

@imkarrer imkarrer commented Apr 22, 2024

Changes

Adds support for custom toml data sources

Context

Closes #25635

Documentation (please check one with an [x])

  • I have updated the documentation, or
  • No documentation update is required

How I've tested my work (please select one)

I have verified these changes via:

  • Code inspection only, or
  • Newly added/modified unit tests, or
  • No unit tests but ran on a real repository, or
  • Both unit tests + ran on a real repository

Happy to hook it up to a real repo but I wasn't sure if I should make one or if there's an existing test fixture.

Closes: #25635

@CLAassistant
Copy link

CLAassistant commented Apr 22, 2024

CLA assistant check
All committers have signed the CLA.

@imkarrer imkarrer changed the title Feat/25635 toml support Feat: 25635 toml support Apr 22, 2024
@imkarrer imkarrer changed the title Feat: 25635 toml support feat: 25635 toml support Apr 22, 2024
@rarkins rarkins changed the title feat: 25635 toml support feat(datasource/custom): add toml support Apr 23, 2024
@rarkins rarkins requested a review from secustor April 23, 2024 03:25
@viceice viceice self-requested a review April 23, 2024 05:59
@@ -316,6 +316,16 @@ export class Http<Opts extends HttpOptions = HttpOptions> {
});
}

async getToml(url: string, options?: Opts): Promise<HttpResponse> {
Copy link
Member

Choose a reason for hiding this comment

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

Needs a separate test in lib/util/http/index.spec.ts

It should also parse the toml and support optional schema validation. please do it like the getJson.
It should be a separate PR.

Copy link
Author

Choose a reason for hiding this comment

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

Gotcha, I understand now how the JSON side works. Just to make sure I understand, you want two PRs.

Copy link
Member

Choose a reason for hiding this comment

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

yes, the split can be done last after this pr

Copy link
Author

Choose a reason for hiding this comment

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

I did some more digging and had some more downtime to look into this.

I may be missing something, but I can't do it precisely like getJson since the internals of lib.util.http delegate to got for parsing the body.

The best I can do is to use a more basic type like text in the InternalHttpOptions.responseType, and then once I have something back from await this.request<ResT>(url, opts) I can parse the TOML and then optionally apply the schema.

Still working on the util.http.index.spec but it feels like I am finally making progress with my understanding of this area.

It appears some of the other JSON support floating around is for interfacing with other systems / clients.

Copy link
Member

Choose a reason for hiding this comment

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

yes, do the parsing in Http class

Copy link
Member

Choose a reason for hiding this comment

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

We should split this file because it's becoming too big. Should of cause be done in a separate PR.

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.

feat(datasource/custom): allow to fetch from TOML data endpoint
3 participants