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

Add inclusive words filter #931

Merged
merged 1 commit into from
Mar 23, 2022
Merged

Add inclusive words filter #931

merged 1 commit into from
Mar 23, 2022

Conversation

DavidOgunsAWS
Copy link
Contributor

Issue #, if available:

  • Adding validator to catch usage of non-inclusive words in the text of a Smithy model

Description of changes:

  • Since the list of non-inclusive words is statically small, those are hard coded
  • The full text scanner internal to the validator can be abstracted further so a single pass over the model runs multiple validators more efficiently.

TODO:
Testing so far has been local. Follow up commit will add checked-in unit tests

By submitting this pull request, I confirm that my contribution is made under the terms of the Apache 2.0 license.

@DavidOgunsAWS DavidOgunsAWS requested a review from a team as a code owner October 8, 2021 18:39
docs/source/1.0/guides/model-linters.rst Outdated Show resolved Hide resolved
:header-rows: 1
:widths: 20 20 60

* - Property
Copy link
Contributor

Choose a reason for hiding this comment

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

Are either of these properties required? What's the default value for appendDefaults if it's not required?

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Will add documentation. It is worth noting there is a non-trivial, maybe counter intuitive defaults behavior:

Though appendDefaults defaults to false, if noninclusiveTerms mappings is entirely unset or empty, appendDefaults behaves as if it were true -- the built in mappings are present. noninclusiveTerms has to be non-empty before appendDefaults behavior applies. If this behavior is acceptable, then I'll focus on clear and concise documentation for it. If not, then I should change the implementation.

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Property structure have changed a bit. But current properties are documented for required or not, along with what the default values are

Copy link
Contributor

@kstich kstich Mar 15, 2022

Choose a reason for hiding this comment

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

Is the reasoning for this structure change because you weren't happy with the behavior? I feel like a terms map and a includeDefaults-like boolean that defaults to true would be pretty clear and usable for customers.

=========

Validates that all text content in a model (i.e. shape names, member names,
documentation, trait values, etc.) do not contain words that perpetuate cultural
Copy link
Contributor

Choose a reason for hiding this comment

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

This looks like it returned or wasn't fixed initially.

:header-rows: 1
:widths: 20 20 60

* - Property
Copy link
Contributor

@kstich kstich Mar 15, 2022

Choose a reason for hiding this comment

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

Is the reasoning for this structure change because you weren't happy with the behavior? I feel like a terms map and a includeDefaults-like boolean that defaults to true would be pretty clear and usable for customers.

Copy link
Contributor

@kstich kstich left a comment

Choose a reason for hiding this comment

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

Let's squash this PR on merge if it's viable.

@DavidOgunsAWS DavidOgunsAWS merged commit 0a0122e into main Mar 23, 2022
@mtdowling mtdowling deleted the wordfilter branch April 8, 2022 05:32
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