Skip to content

Add #lang markdown#59

Open
LeifAndersen wants to merge 1 commit into
greghendershott:masterfrom
LeifAndersen:add-lang
Open

Add #lang markdown#59
LeifAndersen wants to merge 1 commit into
greghendershott:masterfrom
LeifAndersen:add-lang

Conversation

@LeifAndersen

Copy link
Copy Markdown
Contributor

No description provided.

Comment thread markdown/parse.rkt

Copy link
Copy Markdown
Owner

Choose a reason for hiding this comment

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

Thank you very much for the pull request!

I'm reviewing this quickly, not much free time now, so maybe I'm misunderstanding, but:

Wouldn't the new optional argument in-port need to come after the existing optional argument footnote-prefix-symbol? For backward compatibility?

Copy link
Copy Markdown
Contributor Author

Choose a reason for hiding this comment

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

Yes, you are absolutely correct that adding it to the front breaks backwards compatibility. Although for whatever it's worth, changing it to this causes it to follow the read read-syntax etc. convention. Also, afaik, the functions need to have that api to work in the lang/reader.rkt file. However, if you don't want to break backwards compatibility we could always make other functions we use internally that use this api.

It seems like neither frog nor this library rely on that argument, but I'm not sure of what other code does.

Either way is fine with me though. It's a matter of keeping to conventions and reducing complexity, or keeping backwards compatibility.

@greghendershott

Copy link
Copy Markdown
Owner

Pollen uses it.

Actually, I think you simply want to use the existing parse-markdown function instead. It's preferred and will be more efficient with an input-port.

No need to change read-markdown at all; as the comment above it says, it's there only for backward compatibility (ironically :)).

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