bazel/README.md: add aspell comment by daixiang0 · Pull Request #17072 · envoyproxy/envoy

daixiang0 · 2021-06-22T02:33:30Z

Signed-off-by: Long Dai long0dai@foxmail.com

Commit Message:

Add aspell comment.

To fix below error:

root:[envoy]$ ./tools/spelling/check_spelling_pedantic.py fix
Traceback (most recent call last):
  File "./tools/spelling/check_spelling_pedantic.py", line 835, in <module>
    rv = execute(target_paths, args.dictionary, args.operation_type == 'fix')
  File "./tools/spelling/check_spelling_pedantic.py", line 735, in execute
    checker.start()
  File "./tools/spelling/check_spelling_pedantic.py", line 184, in start
    universal_newlines=True)
  File "/usr/lib/python3.6/subprocess.py", line 729, in __init__
    restore_signals, start_new_session)
  File "/usr/lib/python3.6/subprocess.py", line 1364, in _execute_child
    raise child_exception_type(errno_num, err_msg, err_filename)
FileNotFoundError: [Errno 2] No such file or directory: 'aspell': 'aspell'

Additional Description:
Risk Level:
Testing:
Docs Changes:
Release Notes:
Platform Specific Features:
[Optional Runtime guard:]
[Optional Fixes #Issue]
[Optional Deprecated:]
[Optional API Considerations:]

Signed-off-by: Long Dai <long0dai@foxmail.com>

daixiang0 · 2021-06-22T02:33:38Z

/cc @phlax

phlax

@daixiang0 is this the best place to document this ?

phlax · 2021-06-22T13:25:18Z

bazel/README.md

 To run the tools directly, you must install the correct version of clang. This
 may change over time, check the version of clang in the docker image. You must
-also have 'buildifier' installed from the bazel distribution.
+also have 'buildifier' installed from the bazel distribution and `aspell`.


im not sure if this is the correct place to put this - given that currently aspell is required for running the python script and isnt activated via bazel - altho this may change after

I do not find a good place to put it, maybe a new line for it?

i think that information about aspell should not be in this file - there is no bazel job that depends on it - this file is mostly about setting up a bazel environment - info on linting is not relevant here and is wierdly misplaced

that said - its already like that and there is information about running the spell checker below

perhaps add it there - something like "if you wish to run spellchecking please install aspell"

even that tho doesnt really cover it - eg if you install on debian/ubuntu aspell as so apt install --no-install-recommends aspell it wont work correctly - as you need the en dictionaries that are pulled in as recommends

i think this info - like the information about linting - has no place on this page

This section is "Running clang-format without docker", it covers more things indeed, why not rename it as something like "Running format without docker" and comment its dependence like aspell?

rename it as something like "Running format without docker"

my preference would be "Linting your code without docker"

i still think we need to move this section - but agree its beyond the scope of this change

if you want to add information about the aspell dependency - please make it clear that its required for running the check_spelling.py script - not for running the other format tools

Done.

let us do it step by step :)

alyssawilk · 2021-06-22T13:20:32Z

bazel/README.md

 To run the tools directly, you must install the correct version of clang. This
 may change over time, check the version of clang in the docker image. You must
-also have 'buildifier' installed from the bazel distribution.
+also have 'buildifier' installed from the bazel distribution and `aspell`.


actually in retrospect maybe have the same 's around buildifier and aspell?

/wait

sorry I do not get key :(

Signed-off-by: Long Dai <long0dai@foxmail.com>

bazel/README.md

phlax

@daixiang0 a couple of nits then lets land

Signed-off-by: Long Dai <long0dai@foxmail.com>

daixiang0 · 2021-06-28T05:41:19Z

@phlax done.

daixiang0 · 2021-07-05T01:54:15Z

please review @phlax

phlax

lgtm, thanks @daixiang0 - with the caveat, as discussed, that you will need the same aspell dictionaries as ci to correctly spellcheck

* main: listener: match rebalancer to listener IP family type (envoyproxy#16914) jwt_authn: implementation of www-authenticate header (envoyproxy#16216) listener: reset the file event in framework instead of listener filter doing itself (envoyproxy#17227) Small typo fix (envoyproxy#17247) Doc: Clarify request/response attributes are http-only (envoyproxy#17204) bazel/README.md: add aspell comment (envoyproxy#17072) docs: Fix broken URL links in HTTP upgrades doc (envoyproxy#17225) remove the wrong comment on test (envoyproxy#17233) upstream: allow clusters to skip waiting on warmup for initialization (envoyproxy#17179) JwtAuthn: support completing padding on forward jwt payload header (envoyproxy#16752) remove support for v2 UNSUPPORTED_REST_LEGACY (envoyproxy#16968) metrics service: fix wrong argument arrange on MetricsServiceSink (envoyproxy#17127) deps: update cel-cpp to 0.6.1 (envoyproxy#16293) Add ability to filter ConfigDump. (envoyproxy#16774) examples: fix Wasm example. (envoyproxy#17218) upstream: update host's socket factory when metadata is updated. (envoyproxy#16708) Signed-off-by: Garrett Bourg <bourg@squareup.com>

Signed-off-by: Long Dai <long0dai@foxmail.com> Signed-off-by: chris.xin <xinchuantao@qq.com>

Signed-off-by: Long Dai <long0dai@foxmail.com>

bazel/README.md: add aspell comment

c42ec3d

Signed-off-by: Long Dai <long0dai@foxmail.com>

alyssawilk previously approved these changes Jun 22, 2021

View reviewed changes

alyssawilk self-assigned this Jun 22, 2021

phlax requested changes Jun 22, 2021

View reviewed changes

alyssawilk reviewed Jun 22, 2021

View reviewed changes

repokitteh-read-only bot added the waiting label Jun 22, 2021

feedback

92076ab

Signed-off-by: Long Dai <long0dai@foxmail.com>

daixiang0 dismissed alyssawilk’s stale review via 92076ab June 24, 2021 08:20

repokitteh-read-only bot removed the waiting label Jun 24, 2021

alyssawilk assigned phlax and unassigned alyssawilk Jun 24, 2021

phlax reviewed Jun 25, 2021

View reviewed changes

bazel/README.md Outdated Show resolved Hide resolved

phlax reviewed Jun 25, 2021

View reviewed changes

bazel/README.md Outdated Show resolved Hide resolved

phlax requested changes Jun 25, 2021

View reviewed changes

feedback

0377175

Signed-off-by: Long Dai <long0dai@foxmail.com>

phlax approved these changes Jul 5, 2021

View reviewed changes

phlax merged commit a0ca08b into envoyproxy:main Jul 5, 2021

daixiang0 deleted the aspell branch July 6, 2021 01:22

chrisxrepo pushed a commit to chrisxrepo/envoy that referenced this pull request Jul 8, 2021

bazel/README.md: add aspell comment (envoyproxy#17072)

e25b326

Signed-off-by: Long Dai <long0dai@foxmail.com> Signed-off-by: chris.xin <xinchuantao@qq.com>

leyao-daily pushed a commit to leyao-daily/envoy that referenced this pull request Sep 30, 2021

bazel/README.md: add aspell comment (envoyproxy#17072)

e34a5a8

Signed-off-by: Long Dai <long0dai@foxmail.com>

Conversation

daixiang0 commented Jun 22, 2021

Uh oh!

daixiang0 commented Jun 22, 2021

Uh oh!

phlax left a comment

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Uh oh!

Uh oh!

phlax left a comment

Choose a reason for hiding this comment

Uh oh!

daixiang0 commented Jun 28, 2021

Uh oh!

daixiang0 commented Jul 5, 2021

Uh oh!

phlax left a comment

Choose a reason for hiding this comment

Uh oh!

Reviewers

Assignees

Labels

Projects

Milestone

Development

Uh oh!

3 participants