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

Iox #498 remove unreachable code #500

Merged

Conversation

dkroenke
Copy link
Member

@dkroenke dkroenke commented Jan 15, 2021

Pre-Review Checklist for the PR Author

  1. Branch follows the naming format (iox-#123-this-is-a-branch)
  2. Commits messages are according to this guideline
    • Commit messages have the issue ID (iox-#123 commit text)
    • Commit messages are signed (git commit -s)
    • Commit author matches Eclipse Contributor Agreement (and ECA is signed)
  3. Update the PR title
    • Follow the same conventions as for commit messages
    • Link to the relevant issue
  4. Relevant issues are linked
  5. Add sensible notes for the reviewer
  6. All checks have passed
  7. Assign PR to reviewer

Notes for Reviewer

Fix Findings from deepcode.ai

Checklist for the PR Reviewer

  • Commits are properly organized and messages are according to the guideline
  • Code according to our coding style and naming conventions
  • Unit tests have been written for new behavior
  • Public API changes are documented via doxygen
  • Copyright owner are updated in the changed files
  • PR title describes the changes

Post-review Checklist for the PR Author

  1. All open points are addressed and tracked via issues

Post-review Checklist for the Eclipse Committer

  1. All checkboxes in the PR checklist are checked or crossed out
  2. Merge

References

Signed-off-by: Dietrich Krönke <dietrich.kroenke@apex.ai>
Signed-off-by: Dietrich Krönke <dietrich.kroenke@apex.ai>
Signed-off-by: Dietrich Krönke <dietrich.kroenke@apex.ai>
Signed-off-by: Dietrich Krönke <dietrich.kroenke@apex.ai>
@dkroenke dkroenke added the tooling All iceoryx related tooling (scripts etc.) label Jan 15, 2021
README.md Outdated Show resolved Hide resolved
@dkroenke dkroenke changed the title Iox #498 deepcode ai on iceoryx Iox #498 deepcode.ai on iceoryx Jan 15, 2021
Copy link
Member

@elBoberido elBoberido left a comment

Choose a reason for hiding this comment

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

while it's nice to have something like this, I won't be able to fix anything deepcode is complaining about since it requires a login with my github account and I'm not willing to give them my data

iceoryx_posh/source/roudi/roudi_process.cpp Outdated Show resolved Hide resolved
iceoryx_utils/source/posix_wrapper/access_control.cpp Outdated Show resolved Hide resolved
tools/introspection/source/introspection_app.cpp Outdated Show resolved Hide resolved
@dkroenke dkroenke marked this pull request as draft January 15, 2021 18:04
@dkroenke
Copy link
Member Author

while it's nice to have something like this, I won't be able to fix anything deepcode is complaining about since it requires a login with my github account and I'm not willing to give them my data

Ok understood, i see this as a proposal for doing some checks on the code. When the deepcode is inconsistent to other tools and some of the developers will not work with it then we maybe should not take it.
I leave this as draft open for discussion.

@elBoberido
Copy link
Member

It also depends how the errors are made accessible. If it's possible to access them without a login to their service, then it's a non issue.

@dkroenke dkroenke requested a review from marthtz January 18, 2021 10:35
.github/workflows/build-test.yml Show resolved Hide resolved
CONTRIBUTING.md Outdated Show resolved Hide resolved
CONTRIBUTING.md Outdated Show resolved Hide resolved
iceoryx_utils/source/posix_wrapper/access_control.cpp Outdated Show resolved Hide resolved
iceoryx_utils/source/posix_wrapper/access_control.cpp Outdated Show resolved Hide resolved
Signed-off-by: Dietrich Krönke <dietrich.kroenke@apex.ai>
Signed-off-by: Dietrich Krönke <dietrich.kroenke@apex.ai>
Copy link
Contributor

@mossmaurice mossmaurice left a comment

Choose a reason for hiding this comment

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

Thanks for giving deepcode.ai a try! While having more and more badges is of course great, we have to make sure developers are not overwhelmed by warning of several tools. From my experience this leads to most developers ignoring the warnings if there are too many and especially accessible on different websites/services. I'd suggest to pick one or two tools and once developers are used to them, we might continue adding more. As discussed in #409 Axivion will be added in the coming months. @dejanpan suggested VectorCast here. This could be a good replacement for codecov in the future, but of course we have to contact Vector regarding licensing beforehand.

@dkroenke
Copy link
Member Author

It also depends how the errors are made accessible. If it's possible to access them without a login to their service, then it's a non issue.

Unfortunately not i guess, you need to have an account there which can be created via Github to scan and see the results.
I came to the conclusion that we will not have a check on deepcode.ai and i will remove the badge until this is clarified. The other changes can be merged.

Signed-off-by: Dietrich Krönke <dietrich.kroenke@apex.ai>
Signed-off-by: Dietrich Krönke <dietrich.kroenke@apex.ai>
…oryx

Signed-off-by: Dietrich Krönke <dietrich.kroenke@apex.ai>
elBoberido
elBoberido previously approved these changes Jan 22, 2021
Copy link
Member

@elBoberido elBoberido left a comment

Choose a reason for hiding this comment

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

Could you please rename the PR title

@dkroenke dkroenke changed the title Iox #498 deepcode.ai on iceoryx Iox #498 remove unreachable code Jan 22, 2021
@dkroenke dkroenke marked this pull request as ready for review January 22, 2021 15:17
@dkroenke dkroenke added refactoring Refactor code without adding features and removed tooling All iceoryx related tooling (scripts etc.) labels Jan 25, 2021
Signed-off-by: Dietrich Krönke <dietrich.kroenke@apex.ai>
elBoberido
elBoberido previously approved these changes Jan 27, 2021
elBoberido
elBoberido previously approved these changes Jan 27, 2021
@dkroenke dkroenke dismissed elfenpiff’s stale review January 27, 2021 19:05

Review points are integrated

…oryx

Signed-off-by: Dietrich Krönke <dietrich.kroenke@apex.ai>
Signed-off-by: Dietrich Krönke <dietrich.kroenke@apex.ai>
@dkroenke dkroenke merged commit e6172d8 into eclipse-iceoryx:master Jan 29, 2021
@dkroenke dkroenke deleted the iox-#498-deepcode-ai-on-iceoryx branch January 29, 2021 11:13
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
refactoring Refactor code without adding features
Projects
None yet
Development

Successfully merging this pull request may close these issues.

deepcode.ai on iceoryx
5 participants