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 circular dependency & expose a basic namespace map #240

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

Conversation

rescribet
Copy link
Collaborator

@rescribet rescribet commented Jul 11, 2018

The function was using a non-existent variable ns assuming it's available globally (which it might not).

This was taken from the map from link, but adapted and extended with some of the larger vocabs from lov. Notice that map has been frozen to prevent (accidental) change in behaviour by external sources, which is omitted in this commit.

I was wondering whether the following addition would be useful as well;

toJS () {
    return Node.toJS(this)
}

@rescribet
Copy link
Collaborator Author

Oh, I've caused a circular dependency

@rescribet rescribet force-pushed the toJS-ns-undefined-error branch from 95b05fd to 7b4a799 Compare July 11, 2018 16:44
Copy link
Member

@kjetilk kjetilk left a comment

Choose a reason for hiding this comment

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

Looks good to me, only comment I have is that I think mostly the 2007 HTTP ontology is used in Solid, and personally, I prefer the dc prefix for dcterms. I'll let others decide whether to merge.

@rescribet rescribet force-pushed the toJS-ns-undefined-error branch from 7b4a799 to 110a728 Compare November 9, 2018 09:10
@michielbdejong
Copy link
Collaborator

LGTM, can you resolve the conflict?

It was using `ns` assuming it's available globally
@rescribet rescribet force-pushed the toJS-ns-undefined-error branch from 110a728 to 746e15e Compare July 30, 2019 08:25
@rescribet rescribet changed the title Fix node#toJS by exposing a basic namespace map Expose a basic namespace map Jul 30, 2019
@rescribet
Copy link
Collaborator Author

rescribet commented Jul 30, 2019

The conflict has been resolved, I've also added the LDP namespace which was missing

The problem of Node#toJS was fixed in 62da35e, though that created a circular dependency by a top-level import of ns in Node which is still fixed in this commit, so I've renamed the PR to reflect the changes better

@rescribet rescribet changed the title Expose a basic namespace map Fix circular dependency & expose a basic namespace map Jul 30, 2019
@michielbdejong
Copy link
Collaborator

@megoth can you review this PR?

Copy link
Member

@kjetilk kjetilk left a comment

Choose a reason for hiding this comment

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

Actually, I think I'll request some changes now after all this time. I think the patch should be reviewed for consistency with https://github.com/solid/solid-namespace/blob/master/index.js and I think we should review the way we manage these maps, if that implies deprecating solid-namespace in favor doing it in rdflib like here, or if it should be split into modules of its own. But the first stage, IMHO, is to have consistency between this and solid-namespace.

@megoth megoth self-assigned this Aug 5, 2019
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.

4 participants