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

[DISCUSSION] Context class in doc_writer.py #78

Closed
1 of 2 tasks
Purvanshsingh opened this issue May 9, 2021 · 6 comments
Closed
1 of 2 tasks

[DISCUSSION] Context class in doc_writer.py #78

Purvanshsingh opened this issue May 9, 2021 · 6 comments

Comments

@Purvanshsingh
Copy link
Member

Purvanshsingh commented May 9, 2021

I'm submitting a

  • bug report.
  • feature request.

Current Behaviour:

class Context in doc_writer.py is creating context in this format :

Screenshot from 2021-05-09 18-10-48

And we are expecting it in this format:

Screenshot from 2021-05-09 18-11-07

Probably that's why the tests in doc_writer.py are failing

Screenshot from 2021-05-09 18-13-50

I have a concern should we add the base address as well in the context for classes, collection & properties?
"vocab" : base_adderss

Do you want to work on this issue?

yes, If it is required to add.

@farazkhanfk7
Copy link
Member

The format of context was changed in commits by previous contributors and there hasn't been any problem so far. The only problem is failing test cases. I don't think reverting back the changes in doc_writer just to fix the test is a good idea. We should rather update the tests according to the newly made changes. The Context class here is used to generate context for HydraDoc and HydraEntrypoint class. Test cases in core repo should be updated. Also we might consider it's expansion or migration to pytest in future.
See Issue

@Purvanshsingh
Copy link
Member Author

Purvanshsingh commented May 9, 2021

@farazkhanfk7 Yes, changing context is not a good idea for falling tests, I just highlighted the reason for failing.
But I just wanna know why we are not including the base address as vocab: base_address
Because as expected
Class_.title : Class_.id_ will be
"Pet" : "vocab:Pet"
In this case I think adding vocab should be required.

@farazkhanfk7
Copy link
Member

farazkhanfk7 commented May 9, 2021

Yes, even I was thought the same at first when I was working to fix the test cases. But this format works totally fine. These changes were made in this PR where the format of these URIs were changed.

@Purvanshsingh
Copy link
Member Author

Purvanshsingh commented May 10, 2021

@farazkhanfk7 Yes, changing context is not a good idea for falling tests, I just highlighted the reason for failing.
But I just wanna know why we are not including the base address as vocab: base_address
Because as expected
Class_.title : Class_.id_ will be
"Pet" : "vocab:Pet"
In this case I think adding vocab should be required.

@priyanshunayan Can you please help us in understanding this?

@priyanshunayan
Copy link
Member

Hi @Purvanshsingh

But I just wanna know why we are not including the base address as vocab: base_address

We had taken a call to use expanded @id always in order to support as many API Documentations as we could. Different APIDoc authors might have different preferences for eg: why vocab: base_address and why not vocabulary: base_address. They might even use as apidoc: base_address and in those cases, this library will fail to parse those docs. That is why we removed the hard dependency on the vocab keyword and used the expansion algorithm of JSON-LD before parsing the APIDoc and kept the @id expanded. That being said you can always use vocab: base_address in your APIDoc and core/hydrus/agent should work just fine.

Does this answer your question?

@Purvanshsingh
Copy link
Member Author

Purvanshsingh commented May 12, 2021

@priyanshunayan, got your point It makes hydrus more generic to everyone.

Thanks for your help.

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

No branches or pull requests

3 participants