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

Vsri lib crate #95

Merged
merged 2 commits into from
Jul 25, 2024
Merged

Vsri lib crate #95

merged 2 commits into from
Jul 25, 2024

Conversation

worryg0d
Copy link
Collaborator

As discussed in #94, this PR new lib vsri duplicates the implementation of vsri_lib.rs from prometheus-remote crate.

@worryg0d worryg0d added the enhancement New feature or request label Jul 18, 2024
@worryg0d worryg0d self-assigned this Jul 18, 2024
@worryg0d worryg0d force-pushed the vsri-lib-extraction branch 4 times, most recently from 2eb2872 to 8e85022 Compare July 24, 2024 11:24
@worryg0d worryg0d force-pushed the vsri-lib-extraction branch from 8e85022 to 706652a Compare July 24, 2024 11:28
Copy link
Collaborator

@cjrolo cjrolo left a comment

Choose a reason for hiding this comment

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

This seems what is intended! I'm not requesting this to be done now, but probably add a couple of tests as an enhancement for a later PR.

@cjrolo
Copy link
Collaborator

cjrolo commented Jul 24, 2024

Requesting @rukai to look at this.

Copy link
Contributor

@rukai rukai left a comment

Choose a reason for hiding this comment

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

I took a quick look over this, mostly from the perspective of how rust is used.
I noticed a few minor things but nothing impacting correctness.
Looks good!

I also note that csv is an inefficient way of storing integers, but I assume @cjrolo has a good reason for wanting to use csv.

vsri/src/lib.rs Outdated Show resolved Hide resolved
vsri/src/lib.rs Outdated Show resolved Hide resolved
vsri/src/lib.rs Outdated Show resolved Hide resolved
vsri/src/lib.rs Outdated Show resolved Hide resolved
vsri/src/lib.rs Outdated Show resolved Hide resolved
@worryg0d
Copy link
Collaborator Author

@rukai thanks a lot for your feedback! Your suggestions are good and make the general codebase look cleaner here. I'll include them in this PR. Thank you!

Co-authored-by: Lucas Kent <rubickent@gmail.com>
@worryg0d worryg0d force-pushed the vsri-lib-extraction branch from 1d3ffbb to 673d4cf Compare July 25, 2024 11:11
@worryg0d worryg0d merged commit 5816c3f into main Jul 25, 2024
2 checks passed
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
enhancement New feature or request
Projects
None yet
Development

Successfully merging this pull request may close these issues.

3 participants