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

Rename types to use proper PascalCase #79

Merged
merged 1 commit into from
May 17, 2023
Merged

Rename types to use proper PascalCase #79

merged 1 commit into from
May 17, 2023

Conversation

richardpringle
Copy link
Contributor

This adheres to Proper Rust naming conventions

pub fn new(mut receiver: Receiver<Request>, owned_path: PathBuf, cfg: DBConfig) -> Self {
let db = DB::new(owned_path, &cfg).unwrap();
pub fn new(mut receiver: Receiver<Request>, owned_path: PathBuf, cfg: DbConfig) -> Self {
let db = Db::new(owned_path, &cfg).unwrap();
Copy link
Contributor

Choose a reason for hiding this comment

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

Hmm, not sure if DB has to be pascalcase as it is pretty common to use as DB.

Copy link
Contributor Author

Choose a reason for hiding this comment

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

See here

They use Uuid vs UUID as an example. You essentially should never have two uppercase letters in a row for a type name

@@ -77,7 +77,7 @@ pub struct Options {
long,
required = false,
default_value_t = 10,
value_name = "PAYLOAD_MAX_WALK",
value_name = "PAYLOAD_MAX_WalK",
Copy link
Contributor

Choose a reason for hiding this comment

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

Should this follow SCREAMING_SNAKE_CASE or PascalCase style?

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Good catch, whoops!

@richardpringle richardpringle force-pushed the PascalCase branch 3 times, most recently from e8549b9 to 2644149 Compare May 16, 2023 18:10
Copy link
Contributor

@xinifinity xinifinity left a comment

Choose a reason for hiding this comment

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

For new changes, do you mind adding it as a new commit other than a force push? So it is easy for reviewer to just check the diff.

@@ -172,61 +172,61 @@ pub struct Options {
long,
required = false,
default_value_t = 256,
value_name = "DISK_BUFFER_WAL_MAX_AIO_REQUESTS",
help = "Maximum number of concurrent async I/O requests in WAL."
value_name = "DISK_BUFFER_Wal_MAX_AIO_REQUESTS",
Copy link
Contributor

Choose a reason for hiding this comment

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

keep it as SCREAMING_SNAKE_CASE for all other places in this file?

This adheres to Proper Rust naming conventions
@richardpringle
Copy link
Contributor Author

For new changes, do you mind adding it as a new commit other than a force push? So it is easy for reviewer to just check the diff.

The diff is super easy to check with the compare button. I accidentally pushed my "fixup" commit, which I just fixed. That made the compare button behave funny.

But if you ever want to see the actual diff between commits, you just have to use the compare functionality:

https://github.com/ava-labs/firewood/compare/26441491cf206baca4c884a2b823c58da979258c..129378cbdb692a8fad4ba7f232d1f8ebb8b35bd5

It's actually better than going through one commit at a time since I may have changed something in one commit then changed it back in another which would actually make it harder to review.

@xinifinity
Copy link
Contributor

xinifinity commented May 16, 2023

For new changes, do you mind adding it as a new commit other than a force push? So it is easy for reviewer to just check the diff.

The diff is super easy to check with the compare button. I accidentally pushed my "fixup" commit, which I just fixed. That made the compare button behave funny.

But if you ever want to see the actual diff between commits, you just have to use the compare functionality:

https://github.com/ava-labs/firewood/compare/26441491cf206baca4c884a2b823c58da979258c..129378cbdb692a8fad4ba7f232d1f8ebb8b35bd5

It's actually better than going through one commit at a time since I may have changed something in one commit then changed it back in another which would actually make it harder to review.

No worries, I understand sometimes that can happen, happened to me as well :). it's just force push during review seems not a good practice todogroup/gh-issues#53, avoid using it if possible.

@richardpringle richardpringle merged commit a83a96a into main May 17, 2023
@richardpringle richardpringle deleted the PascalCase branch May 26, 2023 19:27
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.

3 participants