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

Cleanup, Arguments #261

Closed
wants to merge 1 commit into from
Closed

Cleanup, Arguments #261

wants to merge 1 commit into from

Conversation

ghost
Copy link

@ghost ghost commented Sep 30, 2021

  • Rewritten the Program.cs to get it clean and readable
  • Rewritten the usage section
  • Added ArgumentParser (!!! ATTENTION !!! some arguments have changed)
    • ProgramArgs contains all possible parameters for the command line
      Feel free to add some ;)

- Rewritten the Program.cs to get it clean and readable
- Rewritten the usage section
- Added ArgumentParser (!!! ATTENTION !!! some arguments have changed)
  - ProgramArgs contains all possible parameters for the command line
    Feel free to add some ;)
@azuisleet azuisleet self-assigned this Sep 30, 2021
@yaakov-h
Copy link
Member

yaakov-h commented Oct 3, 2021

My personal thoughts:

  • I'd heavily prefer to use an existing library, rather than writing our own ad-hoc parsing. If we were going to do it ourselves then I'd also try avoid reflection, as it is unnecessary when you know what all the possibly inputs are.
  • Migrate to System.CommandLine #234 has attempted a similar thing with a different library, but I don't like the way that it represents the options either, and would personally be partial to using CommandLineParser instead.
  • I think its too late in the project to change existing command-line arguments. We've had enough issues with newbies who got confused by the move to a .NET Core Framework-Dependent Deployment, and there seems to be a lot of third-party documentation (forum posts, YouTube videos, etc.) that I would rather not break if there is no reason to.
  • The Pull Request is still a decent amount of work done in good faith, and given the timing I'd be happy to flag it for Hacktoberfest if that's your thing.

@xPaw
Copy link
Member

xPaw commented Apr 26, 2023

I'm gonna close this in favor of #234 as per @yaakov-h's thoughts.

@xPaw xPaw closed this Apr 26, 2023
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