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

add custom namespaces #12

Merged
merged 1 commit into from
Nov 30, 2017
Merged

add custom namespaces #12

merged 1 commit into from
Nov 30, 2017

Conversation

tzapu
Copy link
Contributor

@tzapu tzapu commented Nov 20, 2017

first part of #10

@tzapu
Copy link
Contributor Author

tzapu commented Nov 20, 2017


@tzapu
Copy link
Contributor Author

tzapu commented Nov 21, 2017

hi, the above should fix #10 and #11
if you could take a look @boutros it would be great
for #11 it seems spaces after : were ok, the problem was with tabs and newlines. if you can see a better way to do it please let me know

@boutros
Copy link
Member

boutros commented Nov 21, 2017

Thanks, will review soon!

@boutros
Copy link
Member

boutros commented Nov 23, 2017

I think I would prefer to just export the TripleEncoder.ns map, and avoid expanding the API surface with another exported type.

Is there a reason something like this would not suffice?

3b6ca9d

@tzapu
Copy link
Contributor Author

tzapu commented Nov 24, 2017

hi, that was my first attempt as well, the end result of that if i read it correctly and from what i can remember was that the custom prefixes were not actually written to file (it checks if namespace contains first, and only writes if it is not found, which of course it is because we set it previously)

@tzapu
Copy link
Contributor Author

tzapu commented Nov 24, 2017

but of course, the type can be ommited and you can just use Namespaces as cNS and keep the ns.

i would really really like to somehow keep the flag related to only namespacing if a custom namespace is found, it just makes everything better :D (in our case)

do you want me to modify the pull request as i described and per your instructions?

@tzapu
Copy link
Contributor Author

tzapu commented Nov 28, 2017

hi @boutros , i modified it some more, hopefully it is more along the lines of what you want now

thank you again for looking into this

@boutros
Copy link
Member

boutros commented Nov 30, 2017

Thanks! I'm fine with a flag which turns off auto-generated namespaces.
Could I ask you to squash everything into one commit, but exclude the parts about #11, and open a separate pull-request for that?
If you are not comfortable with rebasing, I will do it, no problem.

@tzapu
Copy link
Contributor Author

tzapu commented Nov 30, 2017

ok, i ll try :d

@tzapu
Copy link
Contributor Author

tzapu commented Nov 30, 2017

i think i sorted it, please take a look,
Thanks

@boutros boutros merged commit b6ee24f into knakk:master Nov 30, 2017
@boutros
Copy link
Member

boutros commented Nov 30, 2017

Great, thanks!

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.

None yet

2 participants