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

GEDCOM: Update tag to record #3386

Merged
merged 3 commits into from Mar 13, 2022
Merged

Conversation

hoonweiting
Copy link
Contributor

In the GEDCOM 5.5.5 specification, tag has been updated to record:

Screenshot

I've kept tag as an alias since some may be using the older specs. However, I'm not sure if string should be kept as an alias, since tag is also a standard token.

@github-actions
Copy link

github-actions bot commented Mar 13, 2022

JS File Size Changes (gzipped)

A total of 1 files have changed, with a combined diff of 0 Bytes (0%).

file master pull size diff % diff
components/prism-gedcom.min.js 239 B 239 B 0 Bytes 0%

Generated by 🚫 dangerJS against 7364e4b

@RunDevelopment
Copy link
Member

However, I'm not sure if string should be kept as an alias, since tag is also a standard token.

Hmmm, we don't disallow this right now, but using two standard tokens can cause problems for themes. If a token uses two standard tokens, then the token with have the color of the CSS rule that is defined last. E.g.

<style>
.token.tag { color: red; }
.token.string { color: blue; }
</style>
<!-- Since `.token.string` is defined last, this token will be blue. -->
<span class="token tag string">foo</span>
<!-- This token will also be blue. -->
<span class="token string tag">bar</span>

This behavior is technically well-defined (it's simply how CSS works), but I doubt that theme authors will expect this.

Honestly, I'm not sure whether using two standard tokens should simply be discouraged and avoided when possible, or whether we should disallow it completely (adding a test for this would be easy). What do you think?

@hoonweiting
Copy link
Contributor Author

That's a good point! I guess I will remove the string alias, although it seems that in all the official themes, .token.string is defined after .token.tag, so I hope that's okay?

Hmm, I'm not really sure whether we should disallow it completely. There is still the question of what we should do with tokens that qualify as both keyword and operator, such as AND and NOT in SQL. However, I guess if we disallow it completely, the question of what to do with overlapping tokens will have a smaller solution space! :P

@RunDevelopment
Copy link
Member

I guess I will remove the string alias, although it seems that in all the official themes, .token.string is defined after .token.tag, so I hope that's okay?

Sounds good to me.

There is still the question of what we should do with tokens that qualify as both keyword and operator, such as AND and NOT in SQL.

Yeah... This will be difficult. If we disallowed multiple standard tokens, then we could just say that operator + keyword is a special standard token. On the other hand, we could also just add a new standard token (e.g. operator-keyword). I'll need some time to think about this some more...

@hoonweiting
Copy link
Contributor Author

I'll need some time to think about this some more...

Alrighty! I wonder how composite tokens would fit into this too, but maybe that's separate enough (non-standard + standard compared to standard + standard that we have here).

Maybe having multiple standard token aliases could be disallowed at least, but I'm not sure if there are any instances where that could make sense yet.

@RunDevelopment RunDevelopment merged commit f8f9534 into PrismJS:master Mar 13, 2022
@hoonweiting hoonweiting deleted the 3170-gedcom branch March 13, 2022 18:11
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