ChainSafe / lodestar

🌟 TypeScript Implementation of Ethereum Consensus
https://lodestar.chainsafe.io
Apache License 2.0
1.12k stars 275 forks source link

Consistent file naming convention #5111

Closed nazarhussain closed 8 months ago

nazarhussain commented 1 year ago

Is your feature request related to a problem? Please describe.

Not having a consistent naming convention cause problems different issues.

  1. Different conventions used by different team members.
  2. Difficult to quickly find a particular file
  3. Sometimes difficulty relating context of piece of code with other

Some examples are:

camelCase https://github.com/ChainSafe/lodestar/blob/unstable/packages/beacon-node/src/chain/blocks/importBlock.ts https://github.com/ChainSafe/lodestar/blobunstable/packages/reqresp/src/encoders/requestDecode.ts

CapitalCase https://github.com/ChainSafe/lodestar/blob/unstable/packages/reqresp/src/ReqResp.ts https://github.com/ChainSafe/lodestar/blob/unstable/packages/beacon-node/src/chain/clock/LocalClock.ts

Describe the solution you'd like

I would like to recommend to have consistent naming convention which follow these rules.

  1. We should strictly use snake_case for file names
  2. If file exports one class, the file name should be the class name in snake_case
  3. If file exports multiple classes then we should split it to different files.
  4. If file exports multiple functions, then we should have a generic name in snake_case
  5. All tests files should have name related to top level describe in snake_case.
  6. All directories should follow snake_case names.

Describe alternatives you've considered

Other approach was to follow camelCase which is already followed on most of the files. The problems with that approach are following:

  1. Windows OS does have case-insensitive file system. So having camelCase reduces our target community for contribution and can cause unseen problem on Windows OS.
  2. Same applies to Mac OS which is more widely used development platform.
  3. camelCase does not have better readability and also does not match the export names that we have in CapitalCase.

Additional context

IN many different softwares, there is a known set of observations and errors that caused due to not handling the proper case sensitive file names. Please check the list in following link.

https://cwe.mitre.org/data/definitions/178.html https://cwe.mitre.org/data/definitions/19.html

nflaig commented 1 year ago

All directories should follow snake_case names.

For directories, I think it is better to use hyphens (hyphen-case) as this matches the naming convention of github projects which are just folders when cloned.

Also I think general naming convention for npm packages is to use hyphens, should probably make sure folder names correspond to package names (without @lodestar/ prefix).

Making files and folders distinguishable based on their naming convention might also provide more clarity.

wemeetagain commented 1 year ago

Understand the sentiment of wanting consistency, but imo this isn't a particularly urgent, important, or impactful decision.

Not sure about yall, but there's no workflow I do that interacts with file names in any way. I'm doing a project-wide search, or navigating to symbol references or definitions. If I need an import, the IDE writes the import statement for me, filepath and all. If I need to test a specific "file", I'm just using mocha --grep to filter.

With that in mind, I'd prefer less invasive solutions, either leave as is, or keep camel case and apply it more consistently.

philknows commented 1 year ago

I have very limited knowledge on this topic so I consulted chatGPT to help me figure out if there were any real distinct advantages/disadvantages outside of personal preferences. I think the most important thing also is that we all adhere to one standard going forward so we can get consistency across everything we do. The only reason why I weakly favour camelCase is to maintain conventions based on the ecosystem we're developing for and the types of developers we work with/develop for around JS/TS. Plus, if I'm correct we're already doing camelCase for most things. And because outside of "consistency" for developer UX, I'd prefer that we spend less resources migrating to something else if it's only for the addressed concerns above which isn't a critical type problem to address IMO.

Windows OS does have case-insensitive file system. So having camelCase reduces our target community for contribution and can cause unseen problem on Windows OS.

I'm not particularly concerned about this problem only because the Windows dev community is small and most people utilizing Ethereum protocol software at this point are not using the software on Windows based systems. It's a small amount and I believe Windows also has a Linux subsystem now for those users - which I'm not sure if it helps alleviate those concerns at this point.

Same applies to Mac OS which is more widely used development platform.

I believe that macOS can be tweaked to be case-sensitive or not. Which is something the developer can customize for themselves if they're running into case sensitivity issues that bug them.

camelCase does not have better readability and also does not match the export names that we have in CapitalCase.

I think there is an argument to be made that snake_case is easier to read because of the underscores, but I can't say that it would make our lives that much better than actually coming to an agreement on convention, whether it's camelCase or something else.

This is coming from someone that doesn't look at the codebase as often as you guys though, so really, as long as we can agree to one standard, I'm good with it.

nazarhussain commented 1 year ago

Based on team discussion we decided to move with camelCase for now.

philknows commented 8 months ago

We made a decision on this, closing unless there are objections.