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

Review PR #110 #114

Merged
merged 2 commits into from
Aug 10, 2022
Merged

Review PR #110 #114

merged 2 commits into from
Aug 10, 2022

Conversation

jibeee
Copy link
Contributor

@jibeee jibeee commented Aug 3, 2022

Minor changes related to PR #110.

  • "view_tag" and "view_tag_full" are now properly escaped in documentation.
  • I modified the prototype of monero_derive_view_tag for readability.

This app has a lot of legacy code, I think new code should have better standards than the older one.

@fbeutin-ledger
Copy link
Contributor

There is indeed a lot of legacy code
There is a follow-up ticket on our board to implement the testing CI for all devices
The static analysis tools added should really help see code smells quickly and I think a good goal would be to require better standard for all new code and modified code

@fbeutin-ledger
Copy link
Contributor

fbeutin-ledger commented Aug 5, 2022

Does it seem sufficient for you ?

@fbeutin-ledger
Copy link
Contributor

Or we can add a task to cleanup Monero, after all it's an in-house app so latest code standards should be expected

@jibeee
Copy link
Contributor Author

jibeee commented Aug 10, 2022

Does it seem sufficient for you ?

Yes, what I mean is that it is a common (and good) practice for contributors to follow the coding style of a project.

But for that specific app, it looks outdated to me, and I think new contributions could use a better coding style than the current one.

@jibeee jibeee merged commit 1d96de2 into develop Aug 10, 2022
@jibeee jibeee deleted the pr110-review branch August 10, 2022 16:54
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