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

Change #1

Open
wants to merge 8 commits into
base: master
Choose a base branch
from
Open

Change #1

wants to merge 8 commits into from

Conversation

fizzbucket
Copy link

This pull request updates the pandoc struct from a tuple struct to a c-type struct so that a hardcoded api version is no longer required. This will let the crate work with the latest versions of Pandoc. It also adds some more testcases and makes minor changes to 2018 edition idioms.

@elliottslaughter
Copy link
Owner

Hi there and thanks for putting this together.

I notice that there's also a fair amount of restructuring in this commit. Is that intentional, and if so is there any particular reason to favor the new structure?

@dkasak
Copy link

dkasak commented May 23, 2020

I'm curious about this as well as I need support for the latest pandoc-types. Pinging @fizzbucket.

@elliottslaughter, are you primarily referring to the removal of the definition module?

@elliottslaughter
Copy link
Owner

Right, there seem to be four distinct kinds of changes in this commit:

  1. Modernization, e.g. updating to 2018 editition and serde usage.
  2. Updating the definitions to be compatible with newer Pandoc types versions.
  3. Moving all definitions into the root of the crate.
  4. Adding test cases.

I'm happy with all of these except (3), which just seems like an anti-pattern to me. Maybe I'm wrong though. Or maybe we could use definition::* inside of lib.rs so we're not literally moving all the definitions. At any rate, if we're going to consider this piece, I'd want to at least have a discussion about it first, and talk about the tradeoffs.

I don't have time at the moment, but maybe in the next couple of evenings I can try to find time to split out (1), (2) and (4) so they can be committed into the repo separately, which would at least unblock users who want to use newer Pandoc Types versions. And then we can have the API discussion separately.

@elliottslaughter
Copy link
Owner

I was looking at this and realized that the examples directory did not get updated. As far as I can tell, there are no examples of directly creating Pandoc objects in the tests. So that's a use case that isn't being exercised in this PR. It also wasn't clear to me how a user was supposed to get the api_version when creating the Pandoc object.

For now I've just bumped the API version to 1.20. This is sufficient to make my local tests pass.

I would be open to further improvements to require less hard-coding, but I'd like to know how we're going to solve the issue with users needing to know what API version to create documents for.

@dkasak if you pull you should be able to use this with the latest pandoc-types.

@elliottslaughter
Copy link
Owner

Ok, I think I've pulled in all of (1), (2) and (4), and have made a new release on crates.io. Please update your Cargo.toml to specify 0.3 and see if it works for you now.

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.

3 participants