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

Fix typos #1650

Open
wants to merge 1 commit into
base: master
Choose a base branch
from
Open

Fix typos #1650

wants to merge 1 commit into from

Conversation

kianmeng
Copy link

@kianmeng kianmeng commented Oct 27, 2024

Found via codespell -L datas,ser,readd,wont,frameword.

@oberstet
Copy link
Contributor

approved (only needed once), to run the CI ... however, unfortunately, the PR "fixes typos" not only in comments and docstrings, but also in member variable names and similar (identifier) : could you remove any changes to identifiers, only keeping comment/docstring changes?

@kianmeng
Copy link
Author

@oberstet Done.

@om26er
Copy link
Contributor

om26er commented Oct 28, 2024

would probably make sense to integrate codepspell in CI as well

Copy link
Contributor

@oberstet oberstet left a comment

Choose a reason for hiding this comment

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

only 1 change left .. pls see below ... if you disagree, pls say so, np, let's discuss - I am not a native speaker ..

@@ -404,7 +404,7 @@ def generate_token(char_groups: int,
:param chars_per_group: Number of characters per character group (or 1 to return a token with no grouping).
:param chars: Characters to choose from. Default is 27 character subset
of the ISO basic Latin alphabet (see: ``DEFAULT_TOKEN_CHARS``).
:param sep: When separating groups in the token, the separater string.
:param sep: When separating groups in the token, the separated string.
:param lower_case: If ``True``, generate token in lower-case.
Copy link
Contributor

Choose a reason for hiding this comment

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

this is wrong, I think the correct word would be separator (noun)

Copy link
Contributor

@oberstet oberstet Oct 28, 2024

Choose a reason for hiding this comment

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

as in (I think, but I'm non-native): separator is the single character used to split the input string into resulting individual separated string portions / parts / components**

so the former is the "tool" (verb/subject), while the latter is the resulting "material" (object)

Copy link
Contributor

Choose a reason for hiding this comment

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

Yes, "separator" is correct

Copy link
Contributor

Choose a reason for hiding this comment

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

while nitpicking, and looking at the type hints we have

def generate_token(char_groups: int,

is that even "correct"?

I mean sep: Optional[str] = None, allows both strings of length 1, and strings of other lengths, while actually it must be 1 char (if non None) ..

Copy link
Contributor

Choose a reason for hiding this comment

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

mmh, yeah, I know that, but that's not a type declaration, but a run-time type check ...

grafik

Copy link
Contributor

Choose a reason for hiding this comment

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

AFAIK the bolted on Python type system is not robust enough to handle "N-character string" in the type-hinting (only, as per above, via runtime checks).

Copy link
Contributor

Choose a reason for hiding this comment

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

seems to be unavailable, at least today (well, and if the AI doesn't lie/hallucinate;)

grafik

Copy link
Contributor

@oberstet oberstet Oct 28, 2024

Choose a reason for hiding this comment

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

fwiw, Rust:

struct SingleChar(String);

impl SingleChar {
    pub fn new(value: &str) -> Option<Self> {
        if value.chars().count() == 1 {
            Some(SingleChar(value.to_string()))
        } else {
            None // Returns None if the input is not exactly one character
        }
    }
}

fn check_single_char(input: Option<SingleChar>) {
    // No runtime check is necessary; input is already guaranteed to be either
    // a valid `SingleChar` or `None`.
}

C++:

#include <string>
#include <optional>
#include <stdexcept>
#include <iostream>

class SingleChar {
public:
    // Constructor that enforces a single-character constraint
    explicit SingleChar(const std::string& s) {
        if (s.size() != 1) {
            throw std::invalid_argument("String must be exactly one character long.");
        }
        value = s;
    }

    // Getter to retrieve the stored character
    char get() const { return value[0]; }

private:
    std::string value;
};

// Function that accepts either a single character or nothing
void checkSingleChar(const std::optional<SingleChar>& input) {
    // No runtime check needed; we rely on the SingleChar constructor
    if (input) {
        std::cout << "Valid single character: " << input->get() << std::endl;
    } else {
        std::cout << "No input provided." << std::endl;
    }
}

int main() {
    try {
        auto validChar = SingleChar("a");
        checkSingleChar(validChar);  // Valid input

        auto invalidChar = SingleChar("abc");  // Throws exception
        checkSingleChar(invalidChar);
    } catch (const std::invalid_argument& e) {
        std::cerr << "Error: " << e.what() << std::endl;
    }

    checkSingleChar(std::nullopt);  // No input provided
}

untested .. I am too lazy .. it looks "reasonable" though ..

Copy link
Contributor

Choose a reason for hiding this comment

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

I think Haskell would just be:

import Data.Char

my_fn :: Char -> Nothing

Copy link
Contributor

Choose a reason for hiding this comment

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

yeah, definitely very nice example.

truth is, only looking at the type system, Haskell (and formerly ML) are unmatched in their underpinnings as they are based on typed lambda calculus (https://en.wikipedia.org/wiki/Typed_lambda_calculus)

now, the fact that Haskell not only has a solid type foundation, but also nice terse algebraic surface syntax makes it even more interesting.

on the other hand, if you'd ask me for the "best syntax" ... obviously taste comes into play .. and so on, but there is one specific argument I always was attracted to:

LISP does away with any surface syntax, and it has this powerful macro thing, which sits on top of https://en.wikipedia.org/wiki/Homoiconicity as an afterburner.

well, and then there is the real world. countless pretty good Python packages;) get stuff done. batteries included.

anyways, nice comment - kudos @meejah

@oberstet
Copy link
Contributor

would probably make sense to integrate codepspell in CI as well

oh yes, "some" spellchecker, preferably one in wide use within Sphinx / Python packages

in general, I kinda regret not having setup docs with all bells and whistles, and CI test-enforce stuff like correct spelling then

if we had done that from day 1, the hurdle to pull that off now is much bigger

fwiw, in a new project I am working on, I did that (and more CI etc) from day 1 .. TDD .. or TDCI .. e.g. deciding to add and maintain a glossary has paid of hugely (for myself), also the other reference sections which of course are just "best practice" in advanced projects

grafik

Found via `codespell -L datas,ser,readd,wont,frameword`
@oberstet
Copy link
Contributor

rgd

https://github.com/crossbario/autobahn-python/actions/runs/11577829463/job/32239966210?pr=1650

it seems this - no longer - works

https://github.com/crossbario/autobahn-python/blame/master/docs/conf.py#L334


rgd

https://github.com/crossbario/autobahn-python/actions/runs/11577829463/job/32239969168?pr=1650#step:6:513

        Makefile.am: installing 'build-aux/depcomp'
        parallel-tests: installing 'build-aux/test-driver'
        configure: error: No modern nasm or yasm found as required. Nasm should be v2.11.01 or later (v2.13 for AVX512) and yasm should be 1.2.0 or later.
        thread 'main' panicked at /home/runner/.cargo/registry/src/index.crates.io-6f17d22bba15001f/isal-sys-0.5.3+496255c/build.rs:72:17:
        configure failed
        note: run with `RUST_BACKTRACE=1` environment variable to display a backtrace
      warning: build failed, waiting for other jobs to finish...
      💥 maturin failed
        Caused by: Failed to build a native library through cargo
        Caused by: Cargo build finished with "exit status: 101": `env -u CARGO PYO3_ENVIRONMENT_SIGNATURE="pypy-3.9-64bit" PYO3_PYTHON="/opt/hostedtoolcache/PyPy/3.9.19/x64/bin/python" PYTHON_SYS_EXECUTABLE="/opt/hostedtoolcache/PyPy/3.9.19/x64/bin/python" "cargo" "rustc" "--message-format" "json-render-diagnostics" "--manifest-path" "/tmp/pip-install-uuwys2qy/cramjam_a007390a70fa43a58e37a6ed8923bea7/Cargo.toml" "--release" "--lib" "--" "-C" "strip=symbols"`
      Error: command ['maturin', 'pep517', 'build-wheel', '-i', '/opt/hostedtoolcache/PyPy/3.9.19/x64/bin/python', '--compatibility', 'off'] returned non-zero exit status 1
      [end of output]

configure: error: No modern nasm or yasm found as required. Nasm should be v2.11.01 or later (v2.13 for AVX512) and yasm should be 1.2.0 or later.

wtf. why?

Copy link
Contributor

@oberstet oberstet left a comment

Choose a reason for hiding this comment

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

I'm sorry, it seems hard to get 100% green CI, and I'm pretty sure the remaining issue (pls see my comment) are unrelated, but ...

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.

4 participants