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

Refactor/profanity checks #312

Merged
merged 10 commits into from
Aug 25, 2022
Merged

Conversation

jmwill86
Copy link
Contributor

Fix/refactor based on user feedback on Discord:

After upgrading shuttle to 0.5.0, I get an error when trying to do anything (deploy, status, local run, etc.). It tells me, that the name I set is an invalid project name. It does only contain lowercase letters, however, it contains the letters "ass" inside the Word ('dachterasse') so it might trigger the profanity check by mistake. Could this be fixed?

I've removed the Censor::Zealous words as they didn't seem overly terrible in my opinion? I've removed some of the shorter words that might easily be within other words or phrases.

I'm not sure what the below line is/was for, it could be that it's a performance thing and my alternate way isn't ideal for some reason. I'll leave that to someone who knows more than I do on that front! Happy to make the changes though.

static INSTANCE: OnceCell<HashSet<String>> = OnceCell::new();
INSTANCE.get_or_init(|| HashSet::from(["Shuttle.rs".to_string()]));

@oddgrd
Copy link
Contributor

oddgrd commented Aug 21, 2022

Nice! I agree that the Zealous word list is a bit much, causing too many false positives.

Regarding the OnceCell, it's used so that the reserved word HashSet only gets instantiated the first time the is_profanity_free_and_not_reserved function is called, not every time. Here is a relevant thread thread from the Rust users forum (lazy_static offers the same functionality we used once_cell for here, but for a few reasons once_cell is recommended now). 🙂

@jmwill86
Copy link
Contributor Author

I've added Rustrict to replace Censor. The reserved 'Shuttle.rs' is covered by other lowercase and punctuation restrictions so I've removed that but left the test in incase it's an issue in future. I've also removed some unused Crates from the common Cargo.toml, they don't seem to be being used anywhere.

Thanks @oddgrd , it's a big help having some references included there to learn more from 😄

Copy link
Contributor

@chesedo chesedo left a comment

Choose a reason for hiding this comment

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

Just one question about reserved words.

cargo sort --workspace should fix the CI failure 😄

common/src/project.rs Outdated Show resolved Hide resolved
common/src/project.rs Show resolved Hide resolved
common/Cargo.toml Outdated Show resolved Hide resolved
@jmwill86
Copy link
Contributor Author

Reserved words added back in. I've left "shuttle.rs" in for now as an example. Would it be better to just leave it blank? With us not having any reserved words currently there's no examples to add in for testing purposes either.

Copy link
Contributor

@chesedo chesedo left a comment

Choose a reason for hiding this comment

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

Nice fixup @jmwill86! I think keeping shuttle.rs as a reserved word is perfect - we don't want it to be a project name and shouldn't rely on some other check which implicitly covers it

@chesedo chesedo merged commit 850eb2c into shuttle-hq:main Aug 25, 2022
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.

3 participants