Fishtest Coding Style Guide [RFC]

ppigazzini commented 4 years ago

Python

Python Official Style Guide Google Python Style Guide

Tools list:

auto-formatter: Black
sort imports: isort
linting: flake8, pylint
Python application management: pipx

Command line:

black --exclude='env|packages' .
isort --profile black --skip env --skip venv --skip worker/packages .

Python version

pyenv simple Python version management for Linux/WSL
fishtest server: Python 3.12
fishtest worker: Python 3.6

CSS, HTML, Javascript

Google HTML/CSS Style Guide Google Javasript Style Guide

Tools list:

Command line:

npx prettier --write 'server/fishtest/static/{css/*.css,html/*.html,js/*.js}'

Shell

Google Shell Style Guide

vondele commented 4 years ago

I'd leave the choice to those that actually write the code. I've used black https://github.com/psf/black in some hobby projects after a recommendation.

linrock commented 4 years ago

following a style guide would be great. +1 to use a code formatter to set a good example in the codebase. i've used black at work before and it's not bad. worth checking out YAPF to see how they compare.

vondele commented 4 years ago

This would be a quick preview on a blackened version of fishtest https://github.com/vondele/fishtest/tree/fishtestBlack

ppigazzini commented 4 years ago

Autoformat test on this snippet from games.py:

master

def make_player(arg):
return run['args'][arg].split(' ')[0]

while games_remaining > 0:
# Run cutechess-cli binary
cmd = [ cutechess, '-repeat', '-rounds', str(int(games_to_play/2)), '-games', ' 2', '-tournament', 'gauntlet'] + pgnout + \
      ['-site', 'https://tests.stockfishchess.org/tests/view/' + run['_id']] + \
      ['-event', 'Batch %d: %s vs %s' % (task_id, make_player('new_tag'), make_player('base_tag'))] + \
      ['-srand', "%d" % struct.unpack("<L", os.urandom(struct.calcsize("<L")))] + \
      ['-resign', 'movecount=3', 'score=400', '-draw', 'movenumber=34',
       'movecount=8', 'score=20', '-concurrency', str(int(games_concurrency))] + pgn_cmd + \
      ['-engine', 'name=New-'+run['args']['resolved_new'][:7], 'cmd=%s' % (new_engine_name)] + new_options + ['_spsa_'] + \
      ['-engine', 'name=Base-'+run['args']['resolved_base'][:7], 'cmd=%s' % (base_engine_name)] + base_options + ['_spsa_'] + \
      ['-each', 'proto=uci', 'tc=%s' % (scaled_tc)] + nodestime_cmd + threads_cmd + book_cmd

task_status = launch_cutechess(cmd, remote, result, spsa_tuning, games_to_play,
                               tc_limit * games_to_play / min(games_to_play, games_concurrency))
if not task_status.get('task_alive', False):
  break

YAPF (pep8 and google styles)

def make_player(arg):
    return run['args'][arg].split(' ')[0]

while games_remaining > 0:
    # Run cutechess-cli binary
    cmd = [ cutechess, '-repeat', '-rounds', str(int(games_to_play/2)), '-games', ' 2', '-tournament', 'gauntlet'] + pgnout + \
          ['-site', 'https://tests.stockfishchess.org/tests/view/' + run['_id']] + \
          ['-event', 'Batch %d: %s vs %s' % (task_id, make_player('new_tag'), make_player('base_tag'))] + \
          ['-srand', "%d" % struct.unpack("<L", os.urandom(struct.calcsize("<L")))] + \
          ['-resign', 'movecount=3', 'score=400', '-draw', 'movenumber=34',
           'movecount=8', 'score=20', '-concurrency', str(int(games_concurrency))] + pgn_cmd + \
          ['-engine', 'name=New-'+run['args']['resolved_new'][:7], 'cmd=%s' % (new_engine_name)] + new_options + ['_spsa_'] + \
          ['-engine', 'name=Base-'+run['args']['resolved_base'][:7], 'cmd=%s' % (base_engine_name)] + base_options + ['_spsa_'] + \
          ['-each', 'proto=uci', 'tc=%s' % (scaled_tc)] + nodestime_cmd + threads_cmd + book_cmd

    task_status = launch_cutechess(
        cmd, remote, result, spsa_tuning, games_to_play,
        tc_limit * games_to_play / min(games_to_play, games_concurrency))
    if not task_status.get('task_alive', False):
        break

Black

def make_player(arg):
    return run["args"][arg].split(" ")[0]

while games_remaining > 0:
    # Run cutechess-cli binary
    cmd = (
        [
            cutechess,
            "-repeat",
            "-rounds",
            str(int(games_to_play / 2)),
            "-games",
            " 2",
            "-tournament",
            "gauntlet",
        ]
        + pgnout
        + ["-site", "https://tests.stockfishchess.org/tests/view/" + run["_id"]]
        + [
            "-event",
            "Batch %d: %s vs %s"
            % (task_id, make_player("new_tag"), make_player("base_tag")),
        ]
        + ["-srand", "%d" % struct.unpack("<L", os.urandom(struct.calcsize("<L")))]
        + [
            "-resign",
            "movecount=3",
            "score=400",
            "-draw",
            "movenumber=34",
            "movecount=8",
            "score=20",
            "-concurrency",
            str(int(games_concurrency)),
        ]
        + pgn_cmd
        + [
            "-engine",
            "name=New-" + run["args"]["resolved_new"][:7],
            "cmd=%s" % (new_engine_name),
        ]
        + new_options
        + ["_spsa_"]
        + [
            "-engine",
            "name=Base-" + run["args"]["resolved_base"][:7],
            "cmd=%s" % (base_engine_name),
        ]
        + base_options
        + ["_spsa_"]
        + ["-each", "proto=uci", "tc=%s" % (scaled_tc)]
        + nodestime_cmd
        + threads_cmd
        + book_cmd
    )

    task_status = launch_cutechess(
        cmd,
        remote,
        result,
        spsa_tuning,
        games_to_play,
        tc_limit * games_to_play / min(games_to_play, games_concurrency),
    )
    if not task_status.get("task_alive", False):
        break

YAPF is a bit slower than Black on fishtest code base. YAPF takes the same time to process already formatted code, instead Black exit immediately.

time yapf -rip .

real    0m23.068s
user    1m7.609s
sys     0m2.781s

time black .

real    0m11.132s
user    1m18.313s
sys     0m3.219s

I'm not able to replicate these YAPF anomalies pointed out in some (old) comparisons:

idempotence: YAPF(YAPF(file.py)) != YAPF(file.py)
determinism: YAPF(file.py)(t_0) != YAPF(file.py)(t_1)

dav1312 commented 2 years ago

Not sure why but this command will ignore some of the files

npx prettier --check "server/fishtest/static/{css/*.css,html/*.html,js/*[!highlight.diff.min].js}"
Checking formatting...
[warn] server\fishtest\static\css\application.css
[warn] server\fishtest\static\js\spsa_new.js
[warn] Code style issues found in 2 files. Forgot to run Prettier?

npx prettier --check "server/fishtest/static/{css/*.css,js/*.js}"
Checking formatting...
[warn] server\fishtest\static\css\application.css
[warn] server\fishtest\static\js\sprt.js
[warn] server\fishtest\static\js\spsa_new.js
[warn] Code style issues found in 3 files. Forgot to run Prettier?

peregrineshahin commented 2 years ago

also, I don't see a .mak in the formatting command, which formatting there should be done also :/

dav1312 commented 2 years ago

I don't see a .mak in the formatting command

I don't think prettier supports mako templates, either format the scripts individually (copy and paste them here https://prettier.io/playground/) or move them to js files so they can be formatted with the command automatically

ppigazzini commented 2 years ago

I formatted the mako templates by hand, leaving unformatted the Githubissues.

Githubissues is a development platform for aggregating issues.

official-stockfish / fishtest