Skip to content

Fix/simplify some validation steps - #643

Merged
fdwr merged 2 commits into
webmachinelearning:mainfrom
inexorabletash:bugfix-option-validation
Apr 18, 2024
Merged

fdwr merged 2 commits into
webmachinelearning:mainfrom
inexorabletash:bugfix-option-validation

Conversation

@inexorabletash

@inexorabletash inexorabletash commented Apr 16, 2024 •

Copy link
Copy Markdown
Contributor
  • Overzealous conversion of "p of x" to "x's p" mangled some cases of "p of x, y or z"; fix them.
  • When validating optional arguments, shorten substeps by using "its" more consistently rather than restating the option name.
  • A few places were operating on options unconditionally. Make those conditional.
  • When comparing two args (e.g. dataTypes), make the order consistent, i.e. "if filter's dataType is not the same as input's data type".
  • Prefer "equal to" over "the same as" in algorithms.
  • Fix a list literal to use 芦禄

Also, sneak in another case where [EMULATED] text macro can be used.


Preview | Diff

- Overzealous conversion of "p of x" to "x's p" mangled some cases of
  "p of x, y or z"; fix them.

- When validating optional arguments, shorten substeps by using "its"
  more consistently rather than restating the option name.

- A few places were operating on options unconditionally. Make those
  conditional.

- When comparing two args (e.g. dataTypes), make the order consistent,
  i.e. "if filter's dataType is not the same as input's data type".

- Prefer "equal to" over "the same as" in algorithms.

- Fix a list literal to use 芦禄

Also, sneak in another case where [EMULATED] text macro can be used.
@inexorabletash

Copy link
Copy Markdown
Contributor Author

This is a preamble to a change that introduces dataType validation to all ops where necessary - this cleanup work was noticed along the way, and keeps that upcoming change nice and clean.

@inexorabletash

Copy link
Copy Markdown
Contributor Author

Please take a look @huningxin and @fdwr ?

@huningxin huningxin left a comment

Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

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

LGTM, thanks for the fix!

@fdwr fdwr left a comment

Copy link
Copy Markdown
Collaborator

Choose a reason for hiding this comment

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

Small comments, else LGTM. Thanks Josh.

Comment thread index.bs
Comment thread index.bs
Comment thread index.bs
Comment thread index.bs
Comment thread index.bs Outdated

@fdwr fdwr left a comment

Copy link
Copy Markdown
Collaborator

Choose a reason for hiding this comment

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

Thanks Josh.

@fdwr
fdwr merged commit 26982c4 into webmachinelearning:main Apr 18, 2024
github-actions Bot added a commit that referenced this pull request Apr 18, 2024
SHA: 26982c4
Reason: push, by fdwr

Co-authored-by: github-actions[bot] <41898282+github-actions[bot]@users.noreply.github.com>
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