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 brief style guidelines. #5

Merged
merged 5 commits into from
Dec 16, 2024
Merged

Add brief style guidelines. #5

merged 5 commits into from
Dec 16, 2024

Conversation

pelesh
Copy link
Collaborator

@pelesh pelesh commented Apr 9, 2024

This should probably be merged to develop, but for opportunistic convenience, I suggest we merge it here. Since #1 will likely prompt discussion on new style guidelines, let's direct this PR to composer-dev branch.

@pelesh pelesh added the documentation Improvements or additions to documentation label Apr 9, 2024
@pelesh pelesh requested a review from reid-g April 9, 2024 18:42
@pelesh pelesh self-assigned this Apr 9, 2024
@pelesh pelesh changed the base branch from composer-dev to develop April 9, 2024 21:12
@pelesh pelesh changed the base branch from develop to composer-dev April 9, 2024 21:12
CONTRIBUTING.md Outdated Show resolved Hide resolved
CONTRIBUTING.md Outdated Show resolved Hide resolved
CONTRIBUTING.md Outdated Show resolved Hide resolved
@pelesh pelesh changed the base branch from composer-dev to develop May 3, 2024 18:19
@pelesh pelesh changed the base branch from develop to composer-dev May 3, 2024 18:20
@Paulm991 Paulm991 marked this pull request as draft June 26, 2024 14:54
Copy link
Collaborator

@stonecoldhughes stonecoldhughes left a comment

Choose a reason for hiding this comment

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

Looks good. I would like to note that whispers of C++20 have reached even into the Contribution.md file. Perhaps something to revisit in the future.

CONTRIBUTING.md Show resolved Hide resolved
CONTRIBUTING.md Outdated Show resolved Hide resolved
CONTRIBUTING.md Outdated Show resolved Hide resolved
@alexander-novo
Copy link
Collaborator

I think it would also be a good idea to set a (soft) column limit of 120 characters.

@pelesh pelesh changed the base branch from composer-dev to develop November 11, 2024 17:38
@pelesh pelesh force-pushed the composer-style-dev branch from 8eafdd4 to 7eed8e5 Compare November 11, 2024 17:41
@pelesh pelesh marked this pull request as ready for review November 11, 2024 17:54
CONTRIBUTING.md Outdated Show resolved Hide resolved
@alexander-novo alexander-novo mentioned this pull request Dec 3, 2024
@reid-g
Copy link
Collaborator

reid-g commented Dec 13, 2024

Some details that I think should be added for clarification.

Initializing classes with the :. The member initializer list can add some weird issues on readability when there are a large number of them. This document should address this.

Structures localized as parameter inputs. When there are a lot of parameters it becomes a lot more manageable to use structures to handle them. Perhaps add some details on this?

These points come from details in the DistributedGenerator.hpp file.

Also with regards to file names and classes. Perhaps state that files with only one class names must match the name of their file (or something similar).

Otherwise looks good to me.

@alexander-novo
Copy link
Collaborator

I concur with @reid-g's points

@pelesh
Copy link
Collaborator Author

pelesh commented Dec 16, 2024

Structures localized as parameter inputs. When there are a lot of parameters it becomes a lot more manageable to use structures to handle them. Perhaps add some details on this?

This is important issue but somewhat out of scope of this PR. @reid-g would you mind creating a separate GitHub issue with details and examples?

@pelesh pelesh merged commit 9cdd63e into develop Dec 16, 2024
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
documentation Improvements or additions to documentation
Projects
None yet
Development

Successfully merging this pull request may close these issues.

4 participants