8000 feat: improve usability of spell.sh by theseion · Pull Request #3238 · coreruleset/coreruleset · GitHub
[go: up one dir, main page]
More Web Proxy on the site http://driver.im/
Skip to content

feat: improve usability of spell.sh #3238

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.

Sign up for GitHub

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

Jump to bottom
Merged
merged 4 commits into from
Jun 15, 2023
Merged

feat: improve usability of spell.sh #3238

merged 4 commits into from
Jun 15, 2023

Conversation

theseion
Copy link
Contributor
@theseion theseion commented Jun 8, 2023 •
edited
Loading
  • add option parsing
  • check whether spell is even installed
  • add option for machine readable output
  • add option for single file parsing
  • add help text
  • make script executable

Replaces #3233.

This was referenced Jun 8, 2023
Merged
Closed
fzipi
fzipi requested changes Jun 10, 2023
View reviewed changes
Copy link
Member
@fzipi fzipi left a comment

Choose a reason for hiding this comment

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

Nice additions.

franbuehler
franbuehler requested changes Jun 11, 2023
View reviewed changes
Copy link
Contributor
@franbuehler franbuehler left a comment •
edited
Loading

Choose a reason for hiding this comment

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

I tested the script for my usecase and it didn't work:

$ util/fp-finder/spell.sh -p regex-assembly/include/windows-commands.ra
util/fp-finder/spell.sh: line 25: syntax error near unexpected token `done'
util/fp-finder/spell.sh: line 25: `    done'

@theseion
Copy link 8000
Contributor Author

Thanks @fzipi. @franbuehler could you try again? I don't have spell on my machine and can't install it either, so it's a real pain to test.

franbuehler
franbuehler requested changes Jun 11, 2023
View reviewed changes
Copy link
Contributor
@franbuehler franbuehler left a comment •
edited
Loading

Choose a reason for hiding this comment

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

I used the option -p like you proposed in this comment, but it doesn't seem to be valid:

util/fp-finder/spell.sh -p regex-assembly/include/windows-commands.ra
Unknown option -p

Ah, I can use the ra file as argument without passing an option: util/fp-finder/spell.sh regex-assembly/include/windows-commands.ra:

util/fp-finder/spell.sh regex-assembly/include/windows-commands.ra
-> checking windows-commands.ra
   `- found English word: active
   `- found English word: add
   `- found English word: append
arp
   `- found English word: assign
assoc
   `- found English word: at
atmadm
attrib
   `- found English word: attributes
auditpol
autochk
autoconv
autofmt
   `- found English word: automount
bcdboot
bcdedit

Then I thought, I could use -m for machine readable output, but then I see all words, not only the common English words:

util/fp-finder/spell.sh regex-assembly/include/windows-commands.ra -m
active
add
append
arp
assign
assoc
at
atmadm
attrib
attributes
auditpol
autochk
autoconv
autofmt
automount
bcdboot

Did I test and understand this correctly?

@theseion
Copy link
Contributor Author

Thanks. Now only English words should appear in the output again.

dune73
dune73 reviewed Jun 12, 2023
View reviewed changes
@dune73
Copy link
Member
dune73 commented Jun 12, 2023

Other than the changes / clarifications mentioned, this looks good to me and ready to merge. Works as advertised.

@theseion theseion requested review from franbuehler and fzipi June 12, 2023 15:07
fzipi
fzipi approved these changes Jun 12, 2023
View reviewed changes
Copy link
Member
@fzipi fzipi left a comment

Choose a reason for hiding this comment

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

LGTM

@dune73
Copy link
Member
dune73 commented Jun 12, 2023

@franbuehler : feel free to merge when you agree.

franbuehler
franbuehler approved these changes Jun 15, 2023
View reviewed changes
Copy link
Contributor
@franbuehler franbuehler left a comment

Choose a reason for hiding this comment

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

Works as intended. Thanks for the PR, @theseion 🎉

@franbuehler franbuehler merged commit 99c1470 into coreruleset:v4.0/dev Jun 15, 2023
@theseion theseion deleted the improve-spell branch June 18, 2023 12:29
@fzipi fzipi mentioned this pull request Jul 2, 2023
Closed
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Reviewers

@dune73 dune73 dune73 left review comments

@fzipi fzipi fzipi approved these changes

@franbuehler franbuehler franbuehler approved these changes

@theMiddleBlue theMiddleBlue Awaiting requested review from theMiddleBlue

Assignees
No one assigned
Labels
None yet
Projects
None yet
Milestone
No milestone
Development

Successfully merging this pull request may close these issues.
4 participants
@theseion @dune73 @fzipi @franbuehler
0