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

feat: add support for pdf a #606

Merged
merged 8 commits into from
Dec 28, 2024
Merged

Conversation

prog-r-amer
Copy link
Contributor

Hi,

my first rust code ever, if there's something to improve please tell me.
I needed the Compiler option for PDFA2 from typst.

Copy link
Owner

@Myriad-Dreamin Myriad-Dreamin left a comment

Choose a reason for hiding this comment

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

Thank you for submitting the PR. Overall, I don't see much problem inside.

packages/typst.node/src/lib.rs Outdated Show resolved Hide resolved
@prog-r-amer
Copy link
Contributor Author

Thanks for looking at the pull request. Isn't the enum still the better way cause it's typed? I think that's pretty helpful.

@Myriad-Dreamin
Copy link
Owner

Myriad-Dreamin commented Nov 27, 2024

Isn't the enum still the better way cause it's typed? I think that's pretty helpful.

I'm not pretty like to force the typing here, as we might have v1.8, v1.9 and more in future. They sounds not really a list that can be exhausted.

Instead we can provide an enum but I think it is better to be provided as string enum at js/ts side. I believe they would be convenient as well:

enum PdfStandard {
    V_1_7 = "v1.7",
    A_2b = "A2b",
}

The latter one (string enum at js/ts side) should provide an optional typing, and people can avoid using it if they don't like it.

@prog-r-amer
Copy link
Contributor Author

Hi, I updated the pull request. I am not 100% sure if thats what you wanted but that's why there is a reviewer ;)

@Myriad-Dreamin
Copy link
Owner

Hi, I updated the pull request. I am not 100% sure if thats what you wanted but that's why there is a reviewer ;)

Sure, I'll improve usability then and @ you in the later PR.

The final remaining thing is to fix the failed tests. You can run it on your repository's GitHub Action or manually run the command in local.

@prog-r-amer
Copy link
Contributor Author

finally found the time and motivation to fix the pipeline stuff.

@Myriad-Dreamin
Copy link
Owner

I was planning to add my effort in a separated PR, but I find all added tests failed because of 79021d7.

@Myriad-Dreamin Myriad-Dreamin merged commit 533347d into Myriad-Dreamin:main Dec 28, 2024
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