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

Update coding, contribution and moderation guides #125

Merged
merged 1 commit into from
Dec 18, 2021

Conversation

haxscramper
Copy link
Collaborator

Work in progress, update contribution, moderation (GitHub moderation for now, will be expanded later) and style guides.

Copy link
Collaborator Author

@haxscramper haxscramper left a comment

Choose a reason for hiding this comment

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

  • Procedure argument alignment - I don't think it makes a lot of sense to
    try and squash all arguments into as little space as possible, vertical
    positioning makes it easier to read.
template newError*(
    conf: ConfigRef,
    wrongNode: PNode,
    report: SemReport,
    args: seq[PNode] = @[],
    info: TLineInfo = wrongNode.info,
  ): untyped =

and not

template newError*(conf: ConfigRef, wrongNode: PNode, report: SemReport,
                   args: seq[PNode] = @[], info: TLineInfo = wrongNode.info): untyped =

doc/intern.rst Outdated
`tests` the tests for |NimSkull|, including the language
specification under `tests/lang`
============ ===================================================
- ``bin/``, ``build/`` - these directories are empty, but are used when Nim
Copy link
Collaborator Author

Choose a reason for hiding this comment

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

Duplicate explanation in the readme and intern.rst, should only be contained in the intern.rst, and readme one was better formatted, so I moved it here.

Copy link
Collaborator

Choose a reason for hiding this comment

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

I kinda wish we just had markdown, or one source of truth, because I'm pretty sure we want to excerpt and not just link information.

Copy link
Collaborator

Choose a reason for hiding this comment

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

Suggested change
- ``bin/``, ``build/`` - these directories are empty, but are used when Nim
- ``bin/``, ``build/`` - these directories are empty, but are used when |NimSkull|

doc/style_guide.rst Show resolved Hide resolved
doc/contributing.rst Outdated Show resolved Hide resolved
doc/contributing.rst Show resolved Hide resolved
@haxscramper haxscramper marked this pull request as ready for review December 12, 2021 18:04
Copy link
Collaborator

@saem saem left a comment

Choose a reason for hiding this comment

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

Some minor quick fixes, others you might want to defer or not do because they're not great ideas.

doc/contributing.rst Outdated Show resolved Hide resolved
doc/contributing.rst Outdated Show resolved Hide resolved
doc/contributing.rst Show resolved Hide resolved
doc/contributing.rst Outdated Show resolved Hide resolved
doc/contributing.rst Outdated Show resolved Hide resolved
doc/style_guide.rst Show resolved Hide resolved
doc/style_guide.rst Outdated Show resolved Hide resolved
doc/style_guide.rst Show resolved Hide resolved
doc/style_guide.rst Show resolved Hide resolved
doc/style_guide.rst Show resolved Hide resolved
@haxscramper haxscramper force-pushed the code-guides branch 3 times, most recently from 930a69c to e3cb2f9 Compare December 18, 2021 15:25
doc/contributing.rst Outdated Show resolved Hide resolved
doc/contributing.rst Outdated Show resolved Hide resolved
Copy link
Collaborator

@saem saem left a comment

Choose a reason for hiding this comment

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

Few items.

Much improved, I'm good to merge after you've had a look at the suggested changes

doc/contributing.rst Outdated Show resolved Hide resolved
doc/contributing.rst Outdated Show resolved Hide resolved
doc/spec.rst Outdated Show resolved Hide resolved
doc/spec.rst Outdated Show resolved Hide resolved
@haxscramper
Copy link
Collaborator Author

bors r+

@saem
Copy link
Collaborator

saem commented Dec 18, 2021

Bors r+

@bors
Copy link
Contributor

bors bot commented Dec 18, 2021

Already running a review

@bors
Copy link
Contributor

bors bot commented Dec 18, 2021

Build succeeded:

@bors bors bot merged commit 0c7a78a into nim-works:devel Dec 18, 2021
bors bot added a commit that referenced this pull request Dec 22, 2021
127: readme.md: Add link to the contribution guide r=saem a=haxscramper

Forgot to add link to the readme in the contribution guide pr - #125 

Co-authored-by: haxscramper <[email protected]>
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.

None yet

3 participants