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 few essential MD files. #1671

Merged
merged 16 commits into from Jun 19, 2019

Conversation

@KaustubhShamshery
Copy link
Collaborator

commented May 24, 2019

Description

Backward Compatibility

  • This is not a breaking change
  • This is a breaking change

Review Suggestion

Status

Implementation

  • ready for review

Integration Test (Core Team)

  • local machine test
  • small-scale cloud test
@codecov-io

This comment has been minimized.

Copy link

commented May 24, 2019

Codecov Report

Merging #1671 into master will decrease coverage by <.01%.
The diff coverage is n/a.

@@            Coverage Diff             @@
##           master    #1671      +/-   ##
==========================================
- Coverage   33.62%   33.61%   -0.01%     
==========================================
  Files         270      270              
  Lines       32999    32999              
==========================================
- Hits        11097    11094       -3     
- Misses      21902    21905       +3
Impacted Files Coverage Δ
src/libUtils/SWInfo.cpp 90.83% <0%> (-2.3%) ⬇️

Continue to review full report at Codecov.

Legend - Click here to learn more
Δ = absolute <relative> (impact), ø = not affected, ? = missing data
Powered by Codecov. Last update 147b74e...958672f. Read the comment docs.


## 6. Reporting Guidelines

If you are subject to or witness unacceptable behavior, or have any other concerns, please notify any Zilliqa Research members on the Zilliqa [Gitter channel](https://gitter.im/Zilliqa/).

This comment has been minimized.

Copy link
@bb111189

bb111189 May 24, 2019

Member

Zilliqa Research team members

Zilliqa's [Gitter channel]

@@ -0,0 +1,20 @@
### Issue and Steps to Reproduce
<!-- Describe your issue and tell us how to reproduce it (include any useful information). -->

This comment has been minimized.

Copy link
@bb111189

bb111189 May 24, 2019

Member

Add warning message not to post any sensitive information like posting of their own private key

`on hold` or should be a draft.

4. - To build your code with clang-format, use `./build.sh style`
- To build your coude with clang-style, use `./build.sh linter`

This comment has been minimized.

Copy link
@bb111189

bb111189 May 24, 2019

Member

coude -> code

- [ ] Medium
- [ ] Low

### Additional Details

This comment has been minimized.

Copy link
@bb111189

bb111189 May 24, 2019

Member

Operating system

<!--Source: https://github.com/ethereum/aleth/blob/master/CODING_STYLE.md -->
## Code Formatting

Use clang-format tool to format your changes, see [CONTRIBUTING](CONTRIBUTING.md) for details.

This comment has been minimized.

Copy link
@ansnunez

ansnunez May 24, 2019

Contributor

Two sentence fragments. Maybe change , see to . See

If not, they must be marked with explicit `[in progress]` or have "[WIP]" in title.

2. - **Pull requests** which fix a bug should start with `fix/`
- **Pull requests** which add a feature to the codebase should start wiht `feature/`

This comment has been minimized.

Copy link
@ansnunez

ansnunez May 24, 2019

Contributor

Typo: wiht

1. All submitted **Pull Requests** are assumed to be ready for review.
If not, they must be marked with explicit `[in progress]` or have "[WIP]" in title.

2. - **Pull requests** which fix a bug should start with `fix/`

This comment has been minimized.

Copy link
@ansnunez

ansnunez May 24, 2019

Contributor

Are you referring to the title, or the branch name?

4. - To build your code with clang-format, use `./build.sh style`
- To build your coude with clang-style, use `./build.sh linter`

5. Kindly go through to .clang-format and .clang-style to see the format and checks enabled

This comment has been minimized.

Copy link
@ansnunez

ansnunez May 24, 2019

Contributor

through to -> through
And maybe better to highlight the files, i.e., .clang-format instead of .clang-format


#### Actual

#### Work Around (If Any)

This comment has been minimized.

Copy link
@ansnunez

ansnunez May 24, 2019

Contributor

Workaround

## Namespaces

1. No `using namespace` declarations in header files.
2. Preprocessor symbols should be prefixed with the namespace in all-caps and an underscore.

This comment has been minimized.

Copy link
@ansnunez

ansnunez May 24, 2019

Contributor

The example immediately below item 2 is for item 1.
Maybe move the example up, and add a second example for item 2.

@@ -0,0 +1,172 @@
# Zilliqa Coding Style

This comment has been minimized.

Copy link
@ansnunez

ansnunez May 24, 2019

Contributor

Some bullet points have a trailing period, the others don't. Please ensure consistency across this file and the others.

@KaustubhShamshery KaustubhShamshery requested review from bb111189 and ansnunez May 24, 2019
@ansnunez ansnunez added this to PRs in development in Core via automation May 24, 2019
@ansnunez ansnunez moved this from PRs in development to PRs needing review (please help!) in Core May 27, 2019
All other entities' first alpha is lower case.
## Variable prefixes

This comment has been minimized.

Copy link
@ansnunez

ansnunez May 27, 2019

Contributor

How about for global const variables (such as the variables mapped to constant.xml values)?

@@ -0,0 +1,20 @@
---

This comment has been minimized.

Copy link
@ansnunez

ansnunez May 29, 2019

Contributor

Do you need to add the same warning about sensitive info here?

1. {Typename} + {qualifiers} + {name}.
2. Only one per line.
3. Favour declarations close to use; don't habitually declare at top of scope ala C.
4. Always pass non-trivial parameters with a const& suffix.

This comment has been minimized.

Copy link
@ansnunez

ansnunez Jun 3, 2019

Contributor

What is non-trivial in this context? And did you mean const& prefix?

- "Effective Modern C++: 42 Specific Ways to Improve Your Use of C++11 and C++14"
[F.21]: https://github.com/isocpp/CppCoreGuidelines/blob/master/CppCoreGuidelines.md#f21-to-return-multiple-out-values-prefer-returning-a-tuple-or-struct

This comment has been minimized.

Copy link
@ansnunez

ansnunez Jun 3, 2019

Contributor

I think the anchor on the linked page has changed recently. It should be: https://github.com/isocpp/CppCoreGuidelines/blob/master/CppCoreGuidelines.md#Rf-out-multi

- Use to avoid vast and unimportant type declarations.
- However, avoid using auto where type is not immediately obvious from the context, and especially not for arithmetic expressions.
8. If you need to pass more than one boolean to a function, consider using an enum instead
9. Prefer enum class to straight enum.

This comment has been minimized.

Copy link
@ansnunez

ansnunez Jun 3, 2019

Contributor

Format enum class and enum

5. To return multiple "out" values, prefer returning a tuple or struct.
See [F.21].
6. Never use a macro where adequate non-preprocessor C++ can be written.
7. Make use of auto whenever type is clear or unimportant:

This comment has been minimized.

Copy link
@ansnunez

ansnunez Jun 3, 2019

Contributor

Format auto

@@ -0,0 +1,172 @@
# Zilliqa Coding Style

This comment has been minimized.

Copy link
@ansnunez

ansnunez Jun 3, 2019

Contributor

Perhaps we can tag the rules on this page that are automatically checked by our tools during compilation? So it's easier to know which are enforced by tools and which still need to be followed manually by coders.

This comment has been minimized.

Copy link
@KaustubhShamshery

KaustubhShamshery Jun 4, 2019

Author Collaborator

already have given some indication in CONTRIBUTING.md

@ansnunez ansnunez requested a review from hazeem-1986 Jun 3, 2019

---

### Issue and Steps to Reproduce

This comment has been minimized.

Copy link
@hazeem-1986

hazeem-1986 Jun 3, 2019

Separate the issue and Steps to Reproduce

@KaustubhShamshery KaustubhShamshery force-pushed the feature/add_mds branch from 7426b8f to ebd0894 Jun 4, 2019

2. - **Pull requests** which fix a bug should have branch name starting with `fix/`
- **Pull requests** which add a feature to the codebase should have branch name starting with `feature/`
- **Pull requests** which are made by outside contributors should also contain their name.

This comment has been minimized.

Copy link
@sharwell

sharwell Jun 6, 2019

Contributor

💡 pull requests sent by outside contributors will come from a fork, so the user ID of the fork will always appear.

This comment has been minimized.

Copy link
@KaustubhShamshery

KaustubhShamshery Jun 6, 2019

Author Collaborator

yes, sometimes we would give access to contributors to make a branch, in that case.

@@ -0,0 +1,20 @@
# Zilliqa Coding and Review Guidelines

1. All submitted **Pull Requests** are assumed to be ready for review.

This comment has been minimized.

Copy link
@sharwell

sharwell Jun 6, 2019

Contributor

💡 "draft" pull requests are intended to replace other designators. They are not yet a full replacement, but it may be good to at least mention that draft pull requests are considered "in progress" even if the title doesn't say wip.

This comment has been minimized.

Copy link
@KaustubhShamshery

KaustubhShamshery Jun 6, 2019

Author Collaborator

yes, but a draft pull request if it does not have the specific tags, can be started to review, according to our guideline

1. All submitted **Pull Requests** are assumed to be ready for review.
If not, they must be marked with explicit `[in progress]` or have "[WIP]" in title.

2. - **Pull requests** which fix a bug should have branch name starting with `fix/`

This comment has been minimized.

Copy link
@sharwell

sharwell Jun 6, 2019

Contributor

💡 Branch naming styles are somewhat problematic. There is no way to resolve this naming issue without closing a pull request and then opening a new one. I would typically recommend omitting this guideline, and only enforcing it internally (i.e. for users who push branches to the root repository as opposed to creating branches in a fork).

This comment has been minimized.

Copy link
@sharwell

sharwell Jun 9, 2019

Contributor

@KaustubhShamshery obviously you don't have to take my suggestion. I'm just pointing out some observations based on my experiences on GitHub over the past several years. 😄


1. No `using namespace` declarations in header files.

```cpp

This comment has been minimized.

Copy link
@Gnnng

Gnnng Jun 7, 2019

Member

can indent the entire code block by 4 spaces when using code block between bullet points, the rendered view will look better

- License.
2. Use Zilliqa Standardized Guard for header files which is given by https://google.github.io/styleguide/cppguide.html#The__define_Guard.
```

This comment has been minimized.

Copy link
@Gnnng

Gnnng Jun 7, 2019

Member

Same indentation suggestion

CODING_STYLE.md Outdated Show resolved Hide resolved
CODING_STYLE.md Outdated Show resolved Hide resolved
CODING_STYLE.md Outdated Show resolved Hide resolved
CODING_STYLE.md Outdated Show resolved Hide resolved
Capitalization

Co-Authored-By: Gong Deli <Gnnnnng@gmail.com>
Core automation moved this from PRs needing review (please help!) to PRs approved - ready to merge! Jun 11, 2019
@ansnunez ansnunez moved this from PRs approved - ready to merge! to PRs needing review (please help!) in Core Jun 11, 2019
@Gnnng
Gnnng approved these changes Jun 11, 2019
Core automation moved this from PRs needing review (please help!) to PRs approved - ready to merge! Jun 11, 2019
@KaustubhShamshery KaustubhShamshery marked this pull request as ready for review Jun 19, 2019
@ansnunez ansnunez merged commit 4ec1323 into master Jun 19, 2019
1 check passed
1 check passed
Travis CI - Branch Build Passed
Details
Core automation moved this from PRs approved - ready to merge! to PRs done (merged) Jun 19, 2019
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Projects
Core
  
PRs done (merged)
7 participants
You can’t perform that action at this time.