Optional

[jpal91]

Which issue does this fix?

Closes #5

Hello! I discovered this package recently, and I'm a big fan! I was hoping that optional arguments were possible, but saw that there was an issue referencing that hadn't been touched in a while.

I'm not sure if this project is in active development anymore so I decided to implement it based on what I saw in the issue and share it with the community if you're still interested!

Describe the solution

mask-parser

This was the bulk of the change. I altered the parse_command_name... function in parser.rs to look for both parenthesis and brackets. It uses similar logic as before (splitting the string) and essentially just does it twice. It now returns the new struct OptionalArg along with the previous return values.

Additional Changes:

• Added OptionalArg which is identical to RequiredArg
  • This could theoretically be an enum instead of two separate structs, but I decided to keep it as is in case their logic diverged anymore (ie optional with default values)
• Small change to the parse function to accept new values from aforementioned function
• Expanded test in parser.rs to include new field optional_args

mask

• Went back to most every function referencing required_args and mirrored an additional loop for optional_args
  • Most of the loops are identical, but main::get_command_options unwraps to an empty str instead of panicking, making it optional
• Added additional tests in tests/arguments_and_flags_test and small changes to other tests that needed optional_args added to the json

Other notes

• I had to add a php action to the Github actions. MacOS was failing on php not being in path for some reason?
• The order in which required and optional arguments are defined in the maskfile doesn't matter with this implementation which could lead to some weirdness but may be desirable to some? Regardless, with a little additional logic it could be altered

[jacobdeichert]

Thanks for the PR! I plan on reviewing it sometime within the next few days/week when I find the time.

Mask is definitely still in development! I just rarely find time to work on it and in its current form it solves 99% of my day to day needs. Optional args I've wanted for a long time and it just never became a high priority, so I'm looking forwards to getting this reviewed and merged 🙂

[jacobdeichert]

Just pointing out that I think this is odd and I'll look into it further unless you do.

I thought php was installed in GH Actions... so maybe that changed?

[jpal91]

It was very strange and it only seemed to happen on MacOS. If you want to check it out, I ran the CI tests on my forked repo, but this was the output -

command=`CLICOLOR_FORCE="1" name="World" "/Users/runner/work/mask/mask/target/debug/mask" "--maskfile" "/var/folders/3m/p59k4qdj0f17st0gn2cmj3640000gn/T/.tmpDcfjK1/maskfile.md" "php"`
code=1
stdout=```""```
stderr=```"\u{1b}[31mERROR:\u{1b}[0m program \'php\' for executor \'php\' not in PATH\n"```

The Windows failure was my fault, I messed up defining the params in a powershell command, so you can ignore that one.

I tried to do a little searching online, but couldn't come up with anything concrete. It's just really weird...

[jacobdeichert]

So because I use macos-latest, and it's been awhile, that runner image is now macOS 14 instead of 13 which used to come with php.

This table shows there's 2 variations of macOS-latest:

• macos-latest - does not come pre-installed with php
• macos-latest-large - DOES come pre-installed with php

So the solution here is to change this workflow to reference macos-latest-large instead of macos-latest and this will ensure php is available.

Can you do this and remove this setup-php action too?

[jpal91]

So I did update this in my most recent commit. However, I couldn't run the actions in my fork because of the core limit on my pro subscription doesn't cover the large packages since they're over 4 cores, I believe.

I'm not sure why the CI isn't running on your side, though. I'm just seeing that it needs reviewer approval first?

[jacobdeichert]

Ah, I didn't know the larger image requires a paid plan!

I see they are failing and I'll look into it further today. I'll probably make a PR to master so you don't have to worry about this after your branch updates.

[jpal91]

Now we know holding php hostage is what gets GitHub the big money 😆.

Sounds good but if you want me to add in another runner, just let me know, it's not a problem.

[jacobdeichert]

I didn't get around to this yesterday unfortunately.

Now I'm on vacation for the next week, but planning on looking at this again once I'm back 👍

[jpal91]

No worries, enjoy the vacation!

[jacobdeichert]

Thanks! I fixed the php issue in PR #110. Once that merges, please update your branch with master whenever you find the time (you'll have conflicts).

I'm aiming to give this PR a proper review within the next week or so. Apologies for the delays!

[jacobdeichert]

Hey, I did a very quick review and looked into that CI php error with a suggestion on how to fix it properly.

I also noticed the actions didn't run on this PR so I'm hoping once you commit some updates they'll re-run correctly.