Skip to content

Add orthogonal edges feature.#48

Merged
kindaro merged 1 commit intojonascarpay:masterfrom
kindaro:feature-orthogonal-edges
Apr 23, 2025
Merged

Add orthogonal edges feature.#48
kindaro merged 1 commit intojonascarpay:masterfrom
kindaro:feature-orthogonal-edges

Conversation

@kindaro
Copy link
Collaborator

@kindaro kindaro commented Apr 21, 2025

Closes #42.

@kindaro
Copy link
Collaborator Author

kindaro commented Apr 21, 2025

Here is how it looks on the module Test.Reference:

Reference

@jonascarpay
Copy link
Owner

How does this interact with --no-splines? Might it make sense to have a three-way flag/toggle Spline | Straight | Ortho?

@kindaro
Copy link
Collaborator Author

kindaro commented Apr 22, 2025

Good question… I had not thought this through.

It does make sense to have a three-way toggle! Let me fix this.

@kindaro kindaro force-pushed the feature-orthogonal-edges branch from 239d1d4 to a18693a Compare April 22, 2025 06:28
@kindaro
Copy link
Collaborator Author

kindaro commented Apr 22, 2025

Three-way toggle installed!

@kindaro kindaro force-pushed the feature-orthogonal-edges branch from a18693a to c58ac2d Compare April 22, 2025 07:18
@jonascarpay
Copy link
Owner

Ah, I think we should probably write the parser similar to this: https://github.com/jonascarpay/calligraphy/blob/master/src/Calligraphy.hs#L166-L171, that way, the CLI will automatically document the possible flags. Does that make sense or was there a particular reason you did it this way?

@kindaro
Copy link
Collaborator Author

kindaro commented Apr 22, 2025

This is a deep question.

In the ideal world, optparse-applicative would give us a way to say «parse this sum type» and automatically generate help text. But it does not.

I do not think adding a bunch of mutually exclusive flags is the right approach, because generally the expectation is that separate flags can be set independently from one another. When we want to telegraph that several flags are mutually exclusive, we make an option instead.

What I am doing is a common approach. Here is an example of a ReadM parser from ormolu, defined the same way as my parser here.When they define the option, they enumerate all the possible values by hand.

That said, I can do it your way as well. It is up to you to decide!

@jonascarpay
Copy link
Owner

I agree neither way is ideal. Let's stick to your way, but then please add the accepted values to the help text and/or the error message

@kindaro kindaro force-pushed the feature-orthogonal-edges branch from c58ac2d to 37122c4 Compare April 23, 2025 13:08
@kindaro
Copy link
Collaborator Author

kindaro commented Apr 23, 2025

I added the accepted values to the help text and to the error message.

@kindaro kindaro requested a review from jonascarpay April 23, 2025 13:09
Copy link
Owner

@jonascarpay jonascarpay left a comment

Choose a reason for hiding this comment

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

LGTM!

@kindaro kindaro merged commit 3b9c1f5 into jonascarpay:master Apr 23, 2025
40 checks passed
@kindaro kindaro mentioned this pull request Apr 23, 2025
@kindaro
Copy link
Collaborator Author

kindaro commented Apr 23, 2025

I forgot to update the change log… See #51.

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.

Orthogonal edges?

2 participants