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

Some small fixes #4371

Closed
wants to merge 3 commits into from
Closed

Some small fixes #4371

wants to merge 3 commits into from

Conversation

rootkea
Copy link
Contributor

@rootkea rootkea commented Oct 23, 2023

No description provided.

rom1v pushed a commit that referenced this pull request Oct 23, 2023
PR #4371 <#4371>

Signed-off-by: Romain Vimont <rom@rom1v.com>
rom1v pushed a commit that referenced this pull request Oct 23, 2023
PR #4371 <#4371>

Signed-off-by: Romain Vimont <rom@rom1v.com>
@rom1v
Copy link
Collaborator

rom1v commented Oct 23, 2023

Thank you 👍

1st commit: ✔️

  • I added the same change in the implementation file
  • I reworded the commit message for the git 50-72 rule

2nd commit: ✔️

3rd commit: ❌

  • the SC_VECTOR is internal to the implementation, and does not need to be exposed
  • there is already sc_usb_devices_destroy_all() which works on any array of sc_usb_device (including "vectors")

Branch pr4371.

@rootkea
Copy link
Contributor Author

rootkea commented Oct 23, 2023

@rom1v Thanks for the review! So should we declare sc_usb_devices_destroy() as static? That could be another way to fix this warning if we don't want to declare the function prototype:

../app/src/usb/usb.c:97:1: warning: no previous prototype for function 'sc_usb_devices_destroy' [-Wmissing-prototypes]
sc_usb_devices_destroy(struct sc_vec_usb_devices *usb_devices) {

@rootkea
Copy link
Contributor Author

rootkea commented Oct 23, 2023

Also thank you for the git 50-72 rule reference! #TIL :)

rom1v pushed a commit that referenced this pull request Oct 23, 2023
It is only called from the implementation file.

PR #4371 <#4371>

Signed-off-by: Romain Vimont <rom@rom1v.com>
@rom1v
Copy link
Collaborator

rom1v commented Oct 23, 2023

So should we declare sc_usb_devices_destroy() as static?

👍 I added a commit on pr4371: 9ade389

That could be another way to fix this warning

Which compiler do you use? I don't get the warning with gcc or clang.

@rootkea
Copy link
Contributor Author

rootkea commented Oct 23, 2023

I also use clang but I compiled with --warnlevel > 2 (3 or everything not really able to remember at the moment...)

@rom1v
Copy link
Collaborator

rom1v commented Oct 23, 2023

Merged into dev.

@rom1v rom1v closed this Oct 23, 2023
@rootkea
Copy link
Contributor Author

rootkea commented Oct 24, 2023

Hey @rom1v since we haven't tagged the repo with the "hacktoberfest" label, could you please tag this PR with "hacktoberfest-accepted" label? Thanks!
https://hacktoberfest.com/participation/#pr-mr-details

@rom1v
Copy link
Collaborator

rom1v commented Oct 24, 2023

Scrcpy does not participate in hacktoberfest specifically.

In their maintainer page, they say:

Be ready to review pull/merge requests, accepting those that are valid by merging them, leaving an overall approving review, or by adding the “hacktoberfest-accepted” label.

I definitely "left an overall approving review" (since the commits have been merged), it should be sufficient. Let me know if it isn't in the end.

@rootkea
Copy link
Contributor Author

rootkea commented Oct 25, 2023

In their maintainer page, they say:

Be ready to review pull/merge requests, accepting those that are valid by merging them, leaving an overall approving review, or by adding the “hacktoberfest-accepted” label.

Yeah, that's for maintainers who have opted to participate in the hacktoberfest since it's an opt-in program.

There are 2 ways to participate:

  1. Tag the repo with "hacktoberfest" label OR
  2. Tag the individual PRs who are interested in participating in hacktoberfest with the "hacktoberfest-accepted" label. (good option if we don't want our repo to participate as a whole)

Currently, this is what I see since neither we have tagged the repo with "hacktoberfest" nor have we labelled the accepted PRs with "hacktoberfest-accepted" label:
hacktoberfest

They specifically say:

[participating] Your PR/MRs must be in a repo tagged with the “hacktoberfest” topic, or have the “hacktoberfest-accepted” label.

The part you quoted - that's is to check if PR can be accepted as a valid PR for hacktoberfest or not after the participation criteria has been checked okay:

[accepted] Your PR/MRs must be merged, have the “hacktoberfest-accepted” label, or have an overall approving review.

If your PR/MR is being accepted for Hacktoberfest via an overall approving review it must also not be closed.

That being said, I'm perfectly fine if you decide not to label the PR with "hacktoberfest-accepted" label since as a maintainer it's not your burden/responsibility. :-)

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Projects
None yet
Development

Successfully merging this pull request may close these issues.

2 participants