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

Move samples out of piet and into local crate #502

Open
wants to merge 1 commit into
base: main
Choose a base branch
from

Conversation

avitex
Copy link
Contributor

@avitex avitex commented Feb 9, 2022

  • Cargo manifests have been cleaned up a bit
  • piet now no longer includes fonts within the crate published, making it smaller
  • piet::Error::InvalidSampleArgs is removed, which is specific to samples

@avitex
Copy link
Contributor Author

avitex commented Feb 9, 2022

I might end up moving samples to another folder while working on a more comprehensive test suite, but I'll leave it like this for now

@avitex
Copy link
Contributor Author

avitex commented Feb 9, 2022

Note the fonts are uploaded in the crate: https://docs.rs/crate/piet/latest/source/

@cmyr
Copy link
Member

cmyr commented Feb 9, 2022

Okay i think this makes sense, and is actually closer to a design we had previously, where we had a piet-test crate. Maybe let's use that name, instead of 'samples', which sounds like something that might be interesting to users?

I'd also like a README in that folder, just explaining that these are a set of tests files that are run against backends for testing, and that the 'snapshots' submodule contains the expected outputs for different backends.

Otherwise thanks for taking the time to sort this out!

Note the fonts are uploaded in the crate: https://docs.rs/crate/piet/latest/source/

oh interesting that's definitely a mistake, I think they were ignored at some point but maybe they were moved.

@avitex
Copy link
Contributor Author

avitex commented Feb 9, 2022

Awesome, this is the line of thinking I'm going down too. I'll move it into piet-test now and build out some more test stuff in there in following PRs

@avitex
Copy link
Contributor Author

avitex commented Feb 9, 2022

The reason I named it samples for the time being is it is close enough to examples, which I would look at for a sample of what I can output with the library. Moving all the stuff into piet-test hides it away a little more

@cmyr
Copy link
Member

cmyr commented Feb 9, 2022

That's fair, but I think it's going to cause more confusion, since it isn't clear how to run these samples, and people have opened issues about that before. I'd prefer to avoid that confusion.

(I'd also like to move the test-picturer.s file out of examples and into bin for each backend that runs the snapshots; I'd also rename it to "generate_snapshots.rs" or something)

@avitex
Copy link
Contributor Author

avitex commented Feb 9, 2022

(I'd also like to move the test-picture.rs file out of examples and into bin for each backend that runs the snapshots; I'd also rename it to "generate_snapshots.rs" or something)

This was almost exactly the next change I was going to propose ha!

@avitex avitex force-pushed the mv-samples branch 2 times, most recently from 83863c3 to 1acf490 Compare February 13, 2022 08:17
- Cargo manifests have been cleaned up a bit
- `piet` now no longer includes fonts within the crate published, making it smaller
- `piet::Error::InvalidSampleArgs` is removed, which is specific to samples
@avitex
Copy link
Contributor Author

avitex commented Feb 13, 2022

Quick note perhaps for further discussion. If the sample_x/generate_snapshots was to be moved from examples to bin this then means piet-test would have to be a published crate to be an optional dependency referenced within required-features for the bin in Cargo.toml. I instead choose to put them each in piet-test for the time being to remove that dependency.

@cmyr
Copy link
Member

cmyr commented Feb 14, 2022

piet-test is already a published crate... does this change your calculus at all?

@xStrom xStrom added the maintenance cleanup and refactoring label Jun 1, 2022
@xStrom
Copy link
Member

xStrom commented Oct 21, 2024

@avitex any interest in reviving this or should we close it?

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
maintenance cleanup and refactoring
Projects
None yet
Development

Successfully merging this pull request may close these issues.

3 participants