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 files for consistency #989

Merged
merged 29 commits into from
Mar 25, 2020
Merged

Conversation

eddyashton
Copy link
Member

@eddyashton eddyashton commented Mar 24, 2020

Resolves #707.

We have no fixed naming scheme for our file names. This PR adds one - we snake_case.h the basenames (dirs are still namespacelikeconcatenated dirs are also now ./snake_case/snake_case.h).

Opening as a draft so people have a chance to veto if they wish, but I'd like to clear this tonight/tomorrow morning so I don't spend too much time resolving conflicts.

Do we like this style?
Do we prefer oneword for some of our core concepts? eg this PR says ring_buffer.h, key_pair.h, etc, does this go too far?
I've ignored libbyz/ so far - should I lower-case those files in the same PR? This now also renames libbyz files to lower_snake_case. This reordered some includes in breaking ways, so there are additional include statements to correct this.

@achamayou
Copy link
Member

+1 for this convention. Can we have a quick script that checks all source files match that convention? [a-z_]+?

@olgavrou
Copy link
Member

I like this style, I prefer ring_buffer vs ringbuffer just so that it is consistent throughout. If you want to also do libbyz then go for it, but it can comfortably go into a different PR (might be quicker to merge this without libbyz at this point)

@achamayou
Copy link
Member

@eddyashton I suggest we align directories also, at least things like: /src/apps/jsgeneric/js_generic.cpp, as well as public headers, and probably binary artifacts too (js_generic.enclave.so.signed)?

@jumaffre
Copy link
Contributor

LGTM.

@eddyashton
Copy link
Member Author

check-format.sh now checks that filenames are lower_snake_cased. It can't check you've put underscores in sensible places, we'll have to enforce that manually. I've also converted libbyz, rather than adding an exception to it for this check.

@achamayou
Copy link
Member

@eddyashton thank you for taking the time to do this, the repo looks much cleaner now!

src/http/ws_upgrade.h Outdated Show resolved Hide resolved
@eddyashton eddyashton marked this pull request as ready for review March 25, 2020 15:25
@eddyashton eddyashton requested a review from a team as a code owner March 25, 2020 15:25
@codecov-io
Copy link

Codecov Report

Merging #989 into master will increase coverage by 4.43%.
The diff coverage is n/a.

Impacted file tree graph

@@            Coverage Diff             @@
##           master     #989      +/-   ##
==========================================
+ Coverage   63.19%   67.63%   +4.43%     
==========================================
  Files         135      103      -32     
  Lines        8971     8266     -705     
==========================================
- Hits         5669     5590      -79     
+ Misses       3302     2676     -626     
Flag Coverage Δ
#unit_BFT ?
#unit_CFT 67.63% <ø> (?)
Impacted Files Coverage Δ
src/apps/lua_generic/lua_generic.cpp 75.28% <ø> (ø)
src/consensus/ledger_enclave.h 44.44% <ø> (ø)
src/consensus/pbft/pbft_requests.h 0.00% <ø> (ø)
src/consensus/raft/raft.h 76.23% <ø> (ø)
src/consensus/raft/raft_types.h 91.67% <ø> (ø)
src/crypto/crypto_box.h 80.00% <ø> (ø)
src/crypto/symmetric_key.cpp 86.54% <ø> (ø)
src/crypto/symmetric_key.h 54.84% <ø> (ø)
src/ds/champ_map.h 92.54% <ø> (ø)
src/ds/logger.h 49.73% <ø> (ø)
... and 64 more

@ghost
Copy link

ghost commented Mar 25, 2020

file_snake_casing@6436 aka 20200325.32 vs master ewma over 30 builds from 6062 to 6428
images

@eddyashton eddyashton merged commit bfb341b into microsoft:master Mar 25, 2020
@eddyashton eddyashton deleted the file_snake_casing branch March 25, 2020 16:03
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.

File names are inconsistent.h
5 participants