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.

Sign up for GitHub

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

Jump to bottom

move yaml behind feature gate #185

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

move yaml behind feature gate #185

wants to merge 1 commit into from

Conversation

cemoktra
Copy link

@cemoktra cemoktra commented Dec 6, 2024 •
edited
Loading

closes #184

@cemoktra cemoktra changed the title move yaml behind feature gate #184 move yaml behind feature gate Dec 6, 2024
CertainLach
CertainLach requested changes Dec 6, 2024
View reviewed changes
Copy link
Owner

@CertainLach CertainLach left a comment

Choose a reason for hiding this comment

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

This dependency isn't large, why would this be a problem?

Yes, it wasn't updated in 2 years, but it is basically feature-complete, and it has no attack surface to be a security concern

crates/jrsonnet-stdlib/Cargo.toml
# std.regexMatch and other helpers
exp-regex = ["dep:regex", "dep:lru", "dep:rustc-hash"]
yaml = ["dep:serde_yaml_with_quirks"]
Copy link
Owner

Choose a reason for hiding this comment

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

Yaml should be added to default-features,

default = ["yaml"]

But then this feature would need to be forwarded thru jrsonnet-cli/jrsonnet crates, and there is no easy way to opt-out of the features other than default-features=false, making this flag not easily usable.

Copy link
Author

Choose a reason for hiding this comment

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

what is your suggestion?

Copy link
Owner

Choose a reason for hiding this comment

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

It is hard to make proper opt-out features, so I would prefer not to make yaml optional.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Reviewers

@CertainLach CertainLach CertainLach requested changes

Requested changes must be addressed to merge this pull request.

Assignees
No one assigned
Labels
None yet
Projects
None yet
Milestone
No milestone
Development

Successfully merging this pull request may close these issues.

Put serde_yaml_with_quicks dependency behind a feature gate
2 participants
@cemoktra @CertainLach