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

Duplicate fieldset error conflicts with documentation #4

Closed
bb010g opened this issue Dec 12, 2018 · 6 comments
Closed

Duplicate fieldset error conflicts with documentation #4

bb010g opened this issue Dec 12, 2018 · 6 comments

Comments

@bb010g
Copy link

bb010g commented Dec 12, 2018

The analysis.duplicateFieldsetEntryName error seems to conflict with enojs's documentation and API.

Duplicate entries in fieldsets currently always errors out with no way to disable this behavior:

const input = `
languages:
eno = eno notation
json = JavaScript Object Notation
eno = eno notation
`.trim();

if(lastNameInstruction.type === 'FIELDSET') {
if(lastFieldsetEntryNames.has(instruction.name)) {
throw errors.duplicateFieldsetEntryName(context, lastNameInstruction, instruction);
} else {
lastFieldsetEntryNames.add(instruction.name);
}

But Fieldset.elements() would be able to give useful data from its entries if this was allowed to parse, and this example is given as the primary reason for using elements in its documentation:

dialogue:
alice = hi
bob = hey
alice = sup?
bob = nothin'

I think a good compromise would be to start by adding an option that controls for the whole parse whether duplicates error, and perhaps down the line some sort of specification scheme could be added so duplicates can error by default, but selectively can be used.

Alternatively / also, it might be useful in those cases to have entries_associative use array values that are appended to when parsing, so element could error on duplicate elements while normally retrieving the head of the list, but an element_multi method could easily pull out properly ordered values for fieldsets with mostly singular elements but some plural cases.

@simonrepp
Copy link
Member

Ah right, well spotted! Thanks for bringing this up. :)

What happened here is I actually made a mistake when I cranked out the example for the API documentation, because the specification for eno dictates that fieldsets indeed are not allowed to have duplicated fields in any case, so the duplicateFieldsetEntryName error and parsing logic conforms with the specification, but the example I wrote does not, that's where I made the mistake.

"Note that unlike in sections, no name can appear twice in a fieldset, every one of them needs to be unique." (Documented at https://eno-lang.org/advanced/, perhaps too hidden though as this might indicate)

I'm going to amend the documentation for enojs (and -php, -py, -rb ;)) and offer a correct, spec-compatible example instead. (the language specification is in a frozen release candidate state so there'd be no leverage to change this on another level right now)

On the long run I can say that I have in fact considered lifting the unique name constraint on fieldsets as well (for eno's next and final spec), so that all elements with key-value signature in eno always act like ordered hash structures with allowed duplicate keys and constraints like these are validated and enforced through the API and application logic (as is already the case for section and as you are also proposing as a remedy for fieldsets). However I will have to think this through very carefully because the uniqueness constraint in fieldsets is connected, if i remember correctly possibly even motivated by, copying and (deep) merging logic, which gets funky when you suddenly try to merge one source field into multiple target fields with that name and vice versa, even more so inside a deep merging hierarchy. ;)

@simonrepp
Copy link
Member

Out of further interest: If this came up while you were trying to implement a real-life usecase (unlike my contrived example) that direly would need name duplication in a fieldset and you'd be willing to disclose that here, I'd love to see it! These things help me to make good, reality-based decisions for the next and final eno specification. :) In any case, thanks for the report, very cool. ❤️

@simonrepp
Copy link
Member

Here's the fix for the documentation: eno-lang/eno-lang.org@79b86ba, already deployed.

I'll be moving the further reaching, specification related aspect of this topic to the eno repository issue tracker in the next days, feel free to follow up here meanwhile. :) thanks!

@simonrepp simonrepp self-assigned this Dec 14, 2018
@simonrepp
Copy link
Member

Hey @bb010g,

I just published an RFC on this matter, thinking this through has been some mental heavy lifting, but I'm happy it's out there and curious what will come of it:
https://github.com/eno-lang/eno/blob/master/rfcs-final-spec/hybrid-fieldsets.md

Thanks for the input!

@simonrepp simonrepp removed their assignment Jan 26, 2019
@bb010g
Copy link
Author

bb010g commented Jan 26, 2019

Thanks, that looks great! I didn't end up continuing with Eno in the project that this came up in, but I've still got it in mind for the future, and that RFC about perfectly covers what I'd want it for—saner, good looking textual input for a variety of different, more complex backends.

@simonrepp
Copy link
Member

Very cool, thanks for the feedback, much appreciated!

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Development

No branches or pull requests

2 participants