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

dex draft #29

Merged
merged 28 commits into from
Aug 23, 2024
Merged

dex draft #29

merged 28 commits into from
Aug 23, 2024

Conversation

Christian-MK
Copy link
Contributor

@Christian-MK Christian-MK commented Jul 17, 2024

a test pr with content for dex

This PR proposes 1 new term (dex) while also proposing edits to existing terms.

@Christian-MK
Copy link
Contributor Author

Some thoughts re label order and context.

As of now, the builder uses and orders the following labels:

  _prefLabel
  _definition
  _related
  _additional // to include notes/references?

I have suggested the addition of skos:altLabel and skos:broader

Two thoughts:

  1. When the above are built, the webpage doesn't make the label names visible; this could result in a loss of context. The values for related or broader are floating-- requiring user to intuit relationship.
  2. Is the above the optimal order? Is the order prescribed by SKOS (I read through the primer and didn't see a required order of labels)?

What about:

"skos:prefLabel";
"skos:altLabel";
"skos:broader";
"skos:definition";
"skos:related";
"skos:note" .

Copy link
Contributor

@waalge waalge left a comment

Choose a reason for hiding this comment

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

When the above are built, the webpage doesn't make the label names visible; this could result in a loss of context. The values for related or broader are floating-- requiring user to intuit relationship.

In line with the comment in the review, I didn't flesh out all of skos: Just prefLabel, definition, something else and then everything else goes in the same 'skos' bucket.

The reason being is I think (name, definition) is > 90% of the value and the overhead of full SKOS is too intimidating / time consuming to justify getting the <10% remaining.
If you want to add the handling for additional SKOS terms and customize their rendering, I'm very happy to support it going in (not this PR obvs).

content/dex.ttl Show resolved Hide resolved
templates/entry.ttl Outdated Show resolved Hide resolved
@Christian-MK
Copy link
Contributor Author

A mistake in the description of 497b631
Was meant to say

Addition of full template and renaming of entry to basic

Copy link
Contributor

@waalge waalge left a comment

Choose a reason for hiding this comment

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

Generally good changes, with a few exceptions:

Overall comments:

  • Please keep mandatory fields before optional ones (prefLabel, definition, ... rest)
  • skos:related values are not strings, they are references. Same with other skos
  • Some times there's triple backticks rather than single quotes. Both are valid, but we can choose one.
  • Need a style guide on 'ID'

Don't know if you responded regarding help rendering for the additional skos fields?

content/cer.ttl Show resolved Hide resolved
content/baseQuote.ttl Outdated Show resolved Hide resolved
content/consumer.ttl Show resolved Hide resolved
content/datum.ttl Show resolved Hide resolved
content/feed.ttl Outdated Show resolved Hide resolved
content/federatedOrcfaxNetwork.ttl Outdated Show resolved Hide resolved
content/feedId.ttl Outdated Show resolved Hide resolved
content/feedId.ttl Outdated Show resolved Hide resolved
content/arkly.ttl Show resolved Hide resolved
content/cer.ttl Show resolved Hide resolved
@Christian-MK
Copy link
Contributor Author

Don't know if you responded regarding help rendering for the additional skos fields?

I would love that! but perhaps we should get feedback from other team members prior to committing the work?

content/domain.ttl Outdated Show resolved Hide resolved
@waalge
Copy link
Contributor

waalge commented Jul 19, 2024

I would love that! but perhaps we should get feedback from other team members prior to committing the work?

I don't think there's much to be discussed until there's a solid proposal to put people to say "do you want this"
At present no-one other than yourself has experienced the suboptimalness of the current setup

content/arkly.ttl Outdated Show resolved Hide resolved
content/arweave.ttl Outdated Show resolved Hide resolved
@Christian-MK Christian-MK mentioned this pull request Jul 25, 2024
@Christian-MK
Copy link
Contributor Author

@waalge I believe I have addressed your concerns/feedback. Would you do a final review?

content/feedId.ttl Outdated Show resolved Hide resolved
Copy link
Contributor

@waalge waalge left a comment

Choose a reason for hiding this comment

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

Just some casing divergence

@@ -12,4 +12,4 @@

Each feed type will have its own naming convention.
''' ;
skos:related "feedID".
skos:related :feedID.
Copy link
Contributor

Choose a reason for hiding this comment

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

Copy link
Contributor Author

Choose a reason for hiding this comment

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

thanks for catching this! have gone back through and made consistent according to our style guide

Copy link
Contributor Author

Choose a reason for hiding this comment

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

skos:broader "forex";
skos:related "CEX";
skos:broader :forex;
skos:related :CEX;
Copy link
Contributor

Choose a reason for hiding this comment

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

See comment re:casing

Copy link
Contributor Author

Choose a reason for hiding this comment

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

skos:broader "forex";
skos:related "DEX";
skos:broader :forex;
skos:related :DEX;
Copy link
Contributor

Choose a reason for hiding this comment

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

See comment re:casing

Copy link
Contributor Author

Choose a reason for hiding this comment

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

information.

It is constructed as `{{feed-id}}/{{feed-version}}`
It is constructed as `{{feed-ID}}/{{feed-version}}`
Copy link
Contributor

Choose a reason for hiding this comment

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

Strictly speaking there is no declared code convention for this section of text. Its psuedo code embedded in text. So this is not wrong. But I think its very weird to case this as it is now.
kebab case is generally all lower.

Copy link
Contributor Author

Choose a reason for hiding this comment

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

fixed

@waalge waalge merged commit 95fc5ea into main Aug 23, 2024
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