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

"graphics" feature gate #373

Merged

Conversation

yutannihilation
Copy link
Contributor

This pull request move graphics-related features behind a feature gate because graphics is not what every user needs. I was trying to create a separate crate for graphics, but I found it's a bit difficult to untie the code that are tightly related each other.

Comment on lines +1 to +2
#[cfg(feature = "graphics")]
mod tests {
Copy link
Contributor Author

Choose a reason for hiding this comment

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

The massive diff on this file is just because wrapping the whole code with #[cfg(feature = "graphics")] affected the indentation. Nothing is changed in actual.

CHANGELOG.md Outdated
@@ -52,6 +52,8 @@

- Removed `TryFrom` conversions between `Robj` and `HashMap` for consistency. `List::into_hashmap()` and `List::from_hashmap()` should be used instead.

- Added support for graphics.
Copy link
Member

Choose a reason for hiding this comment

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

Perhaps mention also the feature that needs to be used to get access to graphics stuff?

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Ah, good idea. Sure.

@Ilia-Kosenkov
Copy link
Member

It is time for extendr to get a separate Features section in Readme, it is hard to track all the options we have right now.

@yutannihilation
Copy link
Contributor Author

I was wondering where is the best place to document it. This pull request adds it to the crate doc, but we also have

and I have no idea when to use which....

@Ilia-Kosenkov
Copy link
Member

My preference: a separate PR summarizing features as a list in extendr README and somewhere on the web site, perhaps also on the front page.

@yutannihilation
Copy link
Contributor Author

Sure. This pull request also adds it to the README of extendr-api crate for now. I'll create another PR for the rest.

@Ilia-Kosenkov
Copy link
Member

I stumbled upon this crate serial_test. Have you ever tried it out? I wonder if decorating your graphics tests with #[serial(graphics)] can help with memory issues while still running tests multithreaded?
(Note that crate guarantees that marked test will run in serial, but other tests will run arbitrary, including in parallel to the #[serial()] ones).

@yutannihilation
Copy link
Contributor Author

Yes, I already tried it, but it didn't save me from #370. I guess this part matters

but other tests will run arbitrary

@yutannihilation
Copy link
Contributor Author

But I didn't investigate much, so it might be worth looking it again at some point....

@Ilia-Kosenkov Ilia-Kosenkov added the feature-graphics All things graphics-related label Feb 11, 2022
Copy link
Member

@Ilia-Kosenkov Ilia-Kosenkov left a comment

Choose a reason for hiding this comment

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

I have introduced a feature-graphics label to easily distinguish everything graphics-related from other topics. This PR looks good to me.

@yutannihilation
Copy link
Contributor Author

Thanks! The label is really useful.

@yutannihilation
Copy link
Contributor Author

@andy-thomason
I'm going to merge this in a week. If you have any concern on moving graphics-related code into a feature gate, please comment here (if not, please just ignore).

@yutannihilation
Copy link
Contributor Author

Merging this and will address #378

@yutannihilation yutannihilation merged commit a5cedef into extendr:master Feb 16, 2022
@yutannihilation yutannihilation deleted the feature/graphics-feature-gate branch February 16, 2022 13:01
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
feature-graphics All things graphics-related
Projects
None yet
Development

Successfully merging this pull request may close these issues.

None yet

2 participants