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

fix: event_id_counter not being used in subscribe_to_system_event() #20

Merged
merged 9 commits into from
Sep 16, 2023

Conversation

juliansebline
Copy link
Contributor

This PR fixes the issue #19

@juliansebline juliansebline changed the title Fix system event id counter Fix event_id_counter Sep 14, 2023
Copy link
Contributor

@Gurgel100 Gurgel100 left a comment

Choose a reason for hiding this comment

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

Please fix the formatting.

unsafe {
let system_event_name = std::ffi::CString::new(system_event_name).unwrap();
Copy link
Contributor

Choose a reason for hiding this comment

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

Why did you move this inside the unsafe block?

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Because it's done this way in map_client_event_to_sim_event

Copy link
Contributor

Choose a reason for hiding this comment

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

In my opinionen the less code in the unsafe block the better so could you move it out of there? Also in map_client_event_to_sim_event.

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Roger

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Done. I did it for another function too.

Copy link
Contributor

@Gurgel100 Gurgel100 left a comment

Choose a reason for hiding this comment

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

LGTM

@Gurgel100
Copy link
Contributor

You still need to fix the formatting.

@juliansebline
Copy link
Contributor Author

You still need to fix the formatting.

Which formatting? You mean the version? If yes, I've tried but I don't know how it works.

@Gurgel100
Copy link
Contributor

You still need to fix the formatting.

Which formatting? You mean the version? If yes, I've tried but I don't know how it works.

I mean the formatting of the code (see failing check).

@juliansebline
Copy link
Contributor Author

juliansebline commented Sep 16, 2023

You still need to fix the formatting.

Which formatting? You mean the version? If yes, I've tried but I don't know how it works.

I mean the formatting of the code (see failing check).

I don't see the formatting problem, sorry. I thought it was related to the package version. Can you show me or fix it? Please

@juliansebline juliansebline changed the title Fix event_id_counter fix: event_id_counter not being used in subscribe_to_system_event() Sep 16, 2023
@juliansebline juliansebline merged commit bb588e5 into main Sep 16, 2023
1 check passed
@juliansebline juliansebline deleted the fix_event_id branch September 16, 2023 21:38
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.

2 participants