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

Fixed typing error #511

Open
wants to merge 1 commit into
base: main
Choose a base branch
from
Open

Conversation

jaxoncreed
Copy link
Contributor

@jaxoncreed jaxoncreed commented Aug 10, 2021

Copy link
Contributor

@bourgeoa bourgeoa left a comment

Choose a reason for hiding this comment

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

@jaxoncreed
Not sure this change is correct.
You cannot parse json or jsonld if data is not a valid json. A minimal string is "{}".

@jeff-zucker do you have any idea ?

@jaxoncreed
Copy link
Contributor Author

At least as far as the typings are concerned, options.data is string | undefined, but jsonString is string | undefined | null. All this does is handle the case where jsonString is null.

@jaxoncreed jaxoncreed mentioned this pull request Aug 23, 2021
41 tasks
@jaxoncreed
Copy link
Contributor Author

@bourgeoa. I looked into it a bit more. options.data is allowed to be undefined as defined here https://github.com/linkeddata/rdflib.js/blob/main/src/fetcher.ts#L187. It's optional, so undefined is allowed. Do you think it shouldn't be optional?

@jeff-zucker
Copy link
Contributor

So it is a question of whether we should let the user create an invalid JSON-LD file. If we say || undefined, we are saying - go ahead create an empty JSON-LD file, you'lll be sorry later, but you asked for it. I we say || {} we are creating something that a later GET with accept=json-ld will not fail on.

@jeff-zucker
Copy link
Contributor

I guess my tendency would be to go ahead and supply the {}.

@jeff-zucker
Copy link
Contributor

The intent of the user's request is "create an empty JSON-LD resource". And BTW I mean jsonString || "{}"; - that will create an empty JSON-LD resource.

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.

3 participants