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

Add pre commit config file #9

Conversation

ajinkyaraj-23
Copy link
Collaborator

fixes #8

  • Added pre-commit file
  • Updated clang-format to latest version
  • updated yaml formatting.

@emturner
Copy link
Collaborator

emturner commented Jan 25, 2024

This is a really large diff, and potentially will make it harder to get the changes into the Ledger repo?

If this goes to them eventually, possibly it would be worth splitting this out into a specific diff, that they can replicate, to avoid needing this change to be audited

@ajinkyaraj-23
Copy link
Collaborator Author

The main change here is clang-format style. No concrete change in the code. Only formatting changes. Would it be ok If i highlight in commit message that "Only formatting changes, no code changed". Something like that? #

This is a really large diff, and potentially will make it harder to get the changes into the Ledger repo?

If this goes to them eventually, possibly it would be worth splitting this out into a specific diff, that they can replicate, to avoid needing this change to be audited

Copy link
Collaborator

@spalmer25 spalmer25 left a comment

Choose a reason for hiding this comment

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

The main change here is clang-format style. No concrete change in the code. Only formatting changes. Would it be ok If i highlight in commit message that "Only formatting changes, no code changed". Something like that? #

This is a really large diff, and potentially will make it harder to get the changes into the Ledger repo?
If this goes to them eventually, possibly it would be worth splitting this out into a specific diff, that they can replicate, to avoid needing this change to be audited

Would it be possible to have .pre-commit-config.yaml and still use the old version of clang (v0.11)?

@emturner
Copy link
Collaborator

The main change here is clang-format style. No concrete change in the code. Only formatting changes. Would it be ok If i highlight in commit message that "Only formatting changes, no code changed". Something like that?

I think it should be done - but in an entirely separate PR to Ledger, from the one that implements the DAL stuff if that makes sense?

If the PR as a whole contains both formatting and impl details, it's much harder to review (yes, they can go commit by commit), but even so they would be fully within their rights to ask for a full re-audit, which we'd want to avoid

@ajinkyaraj-23 ajinkyaraj-23 force-pushed the ajinkyaraj-23@4-add-attestation+dal-support branch from 9e8f403 to c16b6e8 Compare January 26, 2024 10:20
@ajinkyaraj-23
Copy link
Collaborator Author

The main change here is clang-format style. No concrete change in the code. Only formatting changes. Would it be ok If i highlight in commit message that "Only formatting changes, no code changed". Something like that?

I think it should be done - but in an entirely separate PR to Ledger, from the one that implements the DAL stuff if that makes sense?

If the PR as a whole contains both formatting and impl details, it's much harder to review (yes, they can go commit by commit), but even so they would be fully within their rights to ask for a full re-audit, which we'd want to avoid

Makes sense, Lets hold on to this PR until we have merged the DAL and tests in ledger repo. Its not that critical.
I will create seperate PR for documentation and makefile updates.

@spalmer25 spalmer25 removed this from the DAL-Support milestone Feb 6, 2024
@spalmer25 spalmer25 mentioned this pull request Mar 1, 2024
5 tasks
@ajinkyaraj-23
Copy link
Collaborator Author

Closing as this is included in #51

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.

3 participants