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: Added Gura support #467

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

Conversation

polarathene
Copy link
Collaborator

Rebased #446 with conflicts resolved. All credit to @mkvolkov for the actual contribution.


Added support for Gura.

Closes: #245

@polarathene polarathene mentioned this pull request Oct 4, 2023
@mkvolkov
Copy link

mkvolkov commented Oct 5, 2023

@polarathene thanks, you can merge your PR

tests/file_gura.rs Outdated Show resolved Hide resolved
Cargo.toml Outdated Show resolved Hide resolved
@polarathene

This comment was marked as outdated.

Comment on lines +75 to +94
fn test_gura_vec() {
let c = Config::builder()
.add_source(File::from_str(
r#"
hosts: [
"alpha",
"omega"
]
"#,
FileFormat::Gura,
))
.build()
.unwrap();

let v = c.get_array("hosts").unwrap();
let mut vi = v.into_iter();
assert_eq!(vi.next().unwrap().into_string().unwrap(), "alpha");
assert_eq!(vi.next().unwrap().into_string().unwrap(), "omega");
assert!(vi.next().is_none());
}
Copy link
Collaborator Author

@polarathene polarathene Oct 6, 2023

Choose a reason for hiding this comment

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

The test suite check is failing here with:

beta-x86_64-unknown-linux-gnu installed - rustc 1.74.0-beta.1 (b5c050feb 2023-10-03)

...

---- test_gura_vec stdout ----
thread 'test_gura_vec' panicked at /home/runner/.cargo/registry/src/index.crates.io-6f17d22bba15001f/gura-0.5.1/src/parser.rs:1241:26:
index out of bounds: the len is 97 but the index is 97
error: test failed, to rerun pass `--test file_gura`

EDIT: I just ran the test suite with the gura crate reverted back to 0.3 and tests pass. Unfortunately there's no release notes / changelog for this crate, so it's unclear what happened or what the migration is 😕

Presumably if their equivalent serde crate was used instead (uses the gura crate), this would be a non-issue 🤷‍♂️

UPDATE: Adapted to serde_gura and no difference. Identified the failure occurs in 0.5.0 onwards. Raised a bug report upstream. Not much I can do until then, other than revert the version back (I'm not sure what meaningful differences the newer releases have introduced due to lack of changelog / release notes).


NOTE: Despite the test suite claiming to pass for the nightly toolchain, it actually did fail too.

mkvolkov and others added 4 commits October 7, 2023 22:35
Signed-off-by: Brennan Kinney <5098581+polarathene@users.noreply.github.com>
Signed-off-by: Brennan Kinney <5098581+polarathene@users.noreply.github.com>
- `ura` => `gura` corrections
- `src/lib.rs` add mention for Gura support

tests(fix): Use `FileFormat::Gura`
chore: Version bump `gura`

Signed-off-by: Brennan Kinney <5098581+polarathene@users.noreply.github.com>
Signed-off-by: Brennan Kinney <5098581+polarathene@users.noreply.github.com>
polarathene

This comment was marked as resolved.

@polarathene
Copy link
Collaborator Author

polarathene commented Oct 7, 2023

Since I don't know how long (or if) the Gura bug report will take to get a response and be resolved, I've raised another PR which further simplifies maintenance of this PR should it go stale for a while.

Ideally the only future change after review approval would be a version bump in Cargo.toml (even that may not be necessary for a bug fix release).

I'll update this PR to reflect that change (if the linked PR is rejected, the equivalent commit here can be reverted). I'll also address the weird 80 char commit message line length limit 🤔 (seems completely unnecessary in 2023 to limit to 80 chars). (Done)


Latest commit 4140214 depends on #472 to function. In the meantime 0b781b8 is usable if anyone wants to reproduce the test failure.

This method seems to be adapted from `format/json.rs:from_json_value()`.
- I adjusted the order of matched types to mirror that of `format/json.rs`.
- `val` => `value`.
- No need to dereference values, in this case we consume the `value` parameter,
  rather than expect a reference to borrow.
  No need to use `ref` or `clone()` to create owned values.
- Potential Improvement: Adopt the iter + collect pattern used by other formats
  instead of the `for` loop used for array and object types here.

Signed-off-by: Brennan Kinney <5098581+polarathene@users.noreply.github.com>
Now adapted to the better approach handled by json5 equivalent method.
- DRY, avoid the value wrapper reptition via storing the match value and 
  using a final return afterwards to value wrap: `Value::new(uri, vk)`.
- Object and Array kinds now adopt the json5 iter + collect approach.

Additionally corrects the `Cargo.toml` feature `ura` to `gura`.
As the feature name conflicts with the equivalent dependency name, 
the feature needs to prefix the dependency with `dep:`, 
removing the need for the package to include `pacakge = "gura"`.

Signed-off-by: Brennan Kinney <5098581+polarathene@users.noreply.github.com>
Signed-off-by: Brennan Kinney <5098581+polarathene@users.noreply.github.com>
- No `.unwrap()` concern now.
- Leverage the common `from_parsed_value` method.
- Greatly simplified format support.

Signed-off-by: Brennan Kinney <5098581+polarathene@users.noreply.github.com>
@Genarito
Copy link

Genarito commented Oct 7, 2023

gura-rs-parser issue was fixed! 🥳

@vrurg
Copy link

vrurg commented Apr 2, 2024

Turns out, another, now fixed, issue may be related to the failing tests.

Would it be possible to revive this PR?

@polarathene
Copy link
Collaborator Author

Would it be possible to revive this PR?

Yes, sorry I'm just stretched for time.

Trying to polish up a PR to Vector project atm, once I've reached that milestone, I'll switch over to config-rs PRs I have here 👍

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.

[feature request]: Support Gura
6 participants