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鈥檒l occasionally send you account related emails.

Already on GitHub? Sign in to your account

Fix #11: improve parsing #57

Open
wants to merge 1 commit into
base: main
Choose a base branch
from

Conversation

yvan-sraka
Copy link
Contributor

No description provided.

app/Foliage/Meta.hs Outdated Show resolved Hide resolved
@andreabedini
Copy link
Member

That's now how I would like to approach this. I would prefer if foliage would give a parsing error. I guess dioptional is too "powerful" and ignores all parsing failures.

@andreabedini
Copy link
Member

I think this is a limitation of tomland. There is no way to describe a field that has a definite value type but can be missing. The only Alternative interface is at the codec level where it basically means either this codec succeeds or not. There's no "optionality" at the matcher level.

I had a go writing a "maybe" version of match, here.

Perhaps, the easiest approach could be to intercept the KeyNotFound and turn it into a success?

@andreabedini
Copy link
Member

I found a related discussion on the tomland repo.

@yvan-sraka yvan-sraka self-assigned this May 3, 2023
@yvan-sraka yvan-sraka linked an issue May 3, 2023 that may be closed by this pull request
@yvan-sraka yvan-sraka marked this pull request as ready for review May 22, 2023 18:25
@andreabedini andreabedini marked this pull request as draft August 11, 2023 05:51
@andreabedini
Copy link
Member

Marking this as draft, let me know when you think it's ready.

app/Foliage/Meta.hs Outdated Show resolved Hide resolved
@yvan-sraka yvan-sraka marked this pull request as ready for review September 5, 2023 06:43
@yvan-sraka yvan-sraka changed the title [WIP] Fix #11: improve parsing Fix #11: improve parsing Sep 5, 2023
Copy link
Member

@andreabedini andreabedini left a comment

Choose a reason for hiding this comment

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

This might work but we need tests make sure it does. Could you rebase on top of #81 and add a couple of tests?

@yvan-sraka yvan-sraka force-pushed the improve-parsing branch 3 times, most recently from 983296c to adcabcd Compare September 22, 2023 09:26
@yvan-sraka
Copy link
Contributor Author

Seems like it does not work as expected https://github.com/input-output-hk/foliage/pull/57/checks?check_run_id=17034377725 :/

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.

Improve parsing
2 participants