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 questionable hacks for NamedNodes with external subjects #1140

Draft
wants to merge 1 commit into
base: master
Choose a base branch
from

Conversation

piemonkey
Copy link
Contributor

Overview

Draft PR as I thought I should put it somewhere... I thought this was necessary for the blackbox tests on the plugin repo, but it seems that isn't the case. It is necessary in order to compare parsing on rdfa.info/play however...

connected issues and PRs:

#1139
lblod/ember-rdfa-editor-lblod-plugins#387
lblod/ember-rdfa-editor-lblod-plugins#388

Setup

N/A

How to test/reproduce

Import the HTML from any article structures into the editor, then compare the exported HTML to the original in rdfa.info/play.

Challenges/uncertainties

Checks PR readiness

  • UI: works on smaller screen sizes
  • UI: feedback for any loading/error states
  • Check cancel/go-back flows
  • Check database state correct when deleting/updating (especially regarding relationships)
  • changelog
  • npm lint
  • no new deprecations

@@ -1297,7 +1304,8 @@ export class RdfaParser<N> {
) {
// Validate IRIs
if (
(subject.termType === 'NamedNode' && subject.value.indexOf(':') < 0) ||
// More hacking... Does including '#' make sense?
(subject.termType === 'NamedNode' && subject.value.indexOf(':') < 0 && subject.value.indexOf('#')) ||
Copy link
Contributor

Choose a reason for hiding this comment

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

I don't think this is necessarily correct, as IRIs do not need to contain a hash symbol, that is just an often used delimiter.

Copy link
Contributor Author

Choose a reason for hiding this comment

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

This isn't requiring them to contain #, the logic is the other way around. If it does not contain : and does not contain #, then it's not an IRI, so ignore it. This is here to catch the case that we have in article structure nodes where the subject is # (or at least, that's what we have after my hack to handle the nodes with resource="#").

Copy link
Contributor

Choose a reason for hiding this comment

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

Ok I see what you mean, was a bit confused. Would if not be necessary to negate the indexOf('#') then?

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Oh, good spot. It was meant to mimic the : test, so subject.value.indexOf('#') < 0. What I wrote is nonsense as it's only false if it's testing a string that starts with #...

@piemonkey piemonkey force-pushed the fix/test-tweaks branch 2 times, most recently from 35fca9d to 6606d47 Compare February 15, 2024 12:50
Base automatically changed from fix/test-tweaks to feat/rdfa-links-ui February 15, 2024 15:30
@piemonkey piemonkey force-pushed the rdfa/external-subject-hack branch from 4cba0a5 to 77b55c7 Compare February 15, 2024 15:54
@abeforgit
Copy link
Member

I have some ideas for this I wanna try

Base automatically changed from feat/rdfa-links-ui to master March 26, 2024 14:21
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