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

Swap JSON parser #19

Merged
merged 30 commits into from
Dec 10, 2024
Merged

Conversation

Matthew-Mosior
Copy link
Contributor

This PR swaps out the JSON parser found in Language.JSON in contrib for @stefan-hoeck's parser-json (idris2-parser).

This also remove the dependency on contrib.

@Matthew-Mosior Matthew-Mosior changed the title [ new ] Swap JSON parser Swap JSON parser Dec 8, 2024
@mattpolzin
Copy link
Owner

The Makefile ought to run locally for you to save the round trip of GitHub CI running, though I don’t mind hitting the button to get it to run again.

@Matthew-Mosior
Copy link
Contributor Author

The Makefile ought to run locally for you to save the round trip of GitHub CI running, though I don’t mind hitting the button to get it to run again.

I apologize, was fairly certain this Makefile would build.

I did make locally first and it built. Not sure what is going on here, but will get this fixed up.

@mattpolzin
Copy link
Owner

Without digging into the changes in much depth yet, my high level critique would be that I assume there's no reason we can't retain totality for those functions you've made partial so I'd like to do that since totality is such a nice guarantee. Even when I don't get totality I almost never opt for partial over covering (again because coverage checking is such a nice benefit offered by the compiler).

@Matthew-Mosior
Copy link
Contributor Author

Without digging into the changes in much depth yet, my high level critique would be that I assume there's no reason we can't retain totality for those functions you've made partial so I'd like to do that since totality is such a nice guarantee. Even when I don't get totality I almost never opt for partial over covering (again because coverage checking is such a nice benefit offered by the compiler).

I agree with that, I'll re-work the partial functions so that they are at least covering.

@Matthew-Mosior
Copy link
Contributor Author

@mattpolzin

I was able to rewrite the typeSpec function using substr in Prelude.Types instead of using the partial strTail function in Data.String, removing all of the partial annotations.

See 47e0188.

@Matthew-Mosior
Copy link
Contributor Author

@mattpolzin

Not sure if you have time, but if this most recent commit doesn't build, would you mind taking a look at the Makefile? I'm sure I'm missing something silly (not a Makefile expert by any means).

@mattpolzin
Copy link
Owner

Sure. These days with pack around having a Makefile is kind of extra credit but I do want to maintain that because I’ve gone through the effort of making the project build without pack as a dependency. Happy to fix any remaining finicky details.

@Matthew-Mosior
Copy link
Contributor Author

Sure. These days with pack around having a Makefile is kind of extra credit but I do want to maintain that because I’ve gone through the effort of making the project build without pack as a dependency. Happy to fix any remaining finicky details.

Awesome, thank you for helping with this!

@mattpolzin mattpolzin force-pushed the swap-json-parser branch 2 times, most recently from ae0cedf to b38bcd7 Compare December 9, 2024 23:43
Copy link
Owner

@mattpolzin mattpolzin left a comment

Choose a reason for hiding this comment

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

Thanks for taking the time to swap out contrib with Stefan's faster JSON library!

I'm still working through the combination of build & test failures as you can probably see but while I work on that there are some stylistic/preferential requests I have for you.

src/Postgres/DB.idr Show resolved Hide resolved
src/Postgres/Data/PostgresTable.idr Outdated Show resolved Hide resolved
src/Postgres/Data/PostgresValue.idr Outdated Show resolved Hide resolved
src/Postgres/LoadTypes.idr Outdated Show resolved Hide resolved
Copy link
Owner

@mattpolzin mattpolzin left a comment

Choose a reason for hiding this comment

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

I think we're there! Thanks again.

@mattpolzin mattpolzin merged commit 666737e into mattpolzin:main Dec 10, 2024
2 checks passed
@Matthew-Mosior
Copy link
Contributor Author

Thanks for taking the time to swap out contrib with Stefan's faster JSON library!

I'm still working through the combination of build & test failures as you can probably see but while I work on that there are some stylistic/preferential requests I have for you.

You're welcome, I'm glad that I am able to contribute!

Sounds good, see my commits below addressing your comments (you had great ideas/requests btw).

@Matthew-Mosior
Copy link
Contributor Author

I think we're there! Thanks again.

Awesome, you're welcome!

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.

2 participants