search

geolexica / tc211-termbase

ISO/TC 211 termbase programs
1 stars 0 forks source link

`termid` is not always integer #1

Closed strogonoff closed 5 years ago

strogonoff commented 5 years ago

In the resulting YAML, termid is sometimes a string, sometimes an integer:

foo@bar:geolexica.com$ find . -name '*.adoc' | xargs fgrep termid
./_concepts/894.adoc:termid: 894
./_concepts/1458.adoc:termid: 1458
./_concepts/206.adoc:termid: '206'
./_concepts/1233.adoc:termid: 1233

Generally an unwelcome inconsistency, it also prevents sorting concepts by term ID in restricted environments such as Liquid templates.

I think a conversion can be added here (not sure whether this is the correct spot and what is the best way to do this in Ruby):

https://github.com/riboseinc/tc211-termbase/blob/46de3c8992fd4efa952e77e235aaedeb772ec627/lib/tc211/termbase/concept.rb#L35

ronaldtse commented 5 years ago

@strogonoff I agree, and that should be the correct place. Let's make them integers then? I'm actually not sure if Jekyll can do Integer sorting...?

strogonoff commented 5 years ago

Liquid allows sorting by a key and if it’s an integer then I assume it’ll sort by the value. I’m +1 on casting each string to integer unless we actually expect non-integer term IDs.

Something like "termid" => id.to_i on the above-mentioned line might work?

strogonoff commented 5 years ago

perhaps "termid" => Integer(id) is better since that’d ensure it fails on bad input

ronaldtse commented 5 years ago

Perhaps -- I kind of remember that Liquid cannot sort Integers. I think that's why I didn't make the sort happen. Can you double check?

strogonoff commented 5 years ago

@ronaldtse I see what you mean, there are reports that everything in Jekyll document frontmatter gets cast to strings. (Although if everything gets cast to string, it’s super confusing why I was getting what seemed like type mismatch error when trying to sort by termid specifically.)

In any case it’s a generic library without any promise of Jekyll support specifically so IMO it’s not Jekyll sites that should be the main motivation to fix this, just general consistency of output. It might or might not make Geolexica work, I wouldn’t put extra priority because of that.

ronaldtse commented 5 years ago

Correct, this is a generic library. If possible, feel free to help fix this 👍

ronaldtse commented 5 years ago

Fixed, but need to add specs. (in #8)