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

Added a Community ID Function to the stdlib #360

Merged
merged 12 commits into from
Aug 15, 2023

Conversation

owen5mith
Copy link
Contributor

@owen5mith owen5mith commented Aug 2, 2023

This PR addresses #359 by adding a Community ID Function. This is my first time writing Rust, so sorry in advance! 😄

One question I did have was around benchmarks. I have seen the benches/stdlib.rs file, should I be adding this function to those benchmarks, and if so, how do I get them to run? I've tried various cargo bench commands but have not managed to get any of the stdlib benchmarks to execute.

@jszwedko jszwedko requested a review from pront August 2, 2023 14:49
@neuronull neuronull added type: feature A value-adding code addition that introduce new functionality. vrl: stdlib Changes to the standard library labels Aug 2, 2023
@pront
Copy link
Contributor

pront commented Aug 2, 2023

Thank you @owen5mith for this PR!

I will do a detailed review shortly but to answer your question about benchmarks, can you try:

cd VRL_REPO_ROOT
cargo bench

Adding it to benches/stdlib.rs would be nice but not strictly necessary for this PR.

Copy link
Contributor

@pront pront left a comment

Choose a reason for hiding this comment

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

@owen5mith: Thank you for submitting this PR!

This mostly looks good.I left some minor comments but most importantly some tests are failing. I will review again as soon as they are fixed.

src/stdlib/community_id.rs Outdated Show resolved Hide resolved
src/stdlib/community_id.rs Outdated Show resolved Hide resolved
owen5mith and others added 3 commits August 6, 2023 19:03
Co-authored-by: Pavlos Rontidis <pavlos.rontidis@gmail.com>
Co-authored-by: Pavlos Rontidis <pavlos.rontidis@gmail.com>
@pront
Copy link
Contributor

pront commented Aug 7, 2023

Hi @owen5mith, we are almost there.

In order to fix some failing workflows, can you cd into the vector repo root and run:

cargo install -f --path vdev
cargo vdev check rust --clippy
# if clippy reports any errors, please fix 
cargo vdev fmt
# this will ensure is the formatting is good
# commit all changes  

@owen5mith
Copy link
Contributor Author

Hey @pront, sorry for the delay, I was waiting for this PR to be merged, which I hope means this can now compile to WASM. This should now be ready for a re-review. I've run clippy against it, however there were no changes.

@pront pront enabled auto-merge (squash) August 14, 2023 13:35
@pront pront disabled auto-merge August 14, 2023 22:34
@pront
Copy link
Contributor

pront commented Aug 14, 2023

Hi @owen5mith, this looks good now!

@owen5mith
Copy link
Contributor Author

Hi @owen5mith, this looks good now!

Thanks, let me know if I need to add anything else to get this merged! 😀

@pront pront merged commit 2a7295b into vectordotdev:main Aug 15, 2023
8 checks passed
@jszwedko
Copy link
Member

Thanks for this contribution @owen5mith !

@owen5mith owen5mith deleted the feature/add-community-id branch August 15, 2023 21:03
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
type: feature A value-adding code addition that introduce new functionality. vrl: stdlib Changes to the standard library
Projects
None yet
Development

Successfully merging this pull request may close these issues.

4 participants