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

--trace option & New parsing with optional output file argument #18

Merged
merged 11 commits into from
Feb 25, 2024

Conversation

YourMJK
Copy link
Contributor

@YourMJK YourMJK commented Feb 7, 2024

See discussion: #17 (comment)

  • Adds a new --trace option for printing structure and contents of segments
  • Makes positional output file argument optional for actions that don't modify the input (like --trace)
  • Replaces SEGFAULTs with standard parsing error when user inputs too few arguments
  • Inline increment of argument index during parsing to reduce code clutter
  • Removes case-insensitivity of options

@MonoS
Copy link
Owner

MonoS commented Feb 7, 2024

Seems fine to me, I'll test it a bit tomorrow and let you know :)

@YourMJK
Copy link
Contributor Author

YourMJK commented Feb 20, 2024

@MonoS Did you come around to testing this? Let me know if you want me to change something.

@MonoS
Copy link
Owner

MonoS commented Feb 21, 2024

Did you come around to testing this?

Yeah, i've put some comment in your code, i can see them in this PR before your latest comment.

@YourMJK
Copy link
Contributor Author

YourMJK commented Feb 21, 2024

Oh, don't know why but I can't see those unfortunately ¯\(ツ)

@MonoS
Copy link
Owner

MonoS commented Feb 23, 2024

Oh, don't know why but I can't see those unfortunately ¯_(ツ)_/¯

That's probably because I don't know how to use github :)

Anyway:

  • The new --trace doesn't have the check for its option without dash
  • In the WDS you print both position and size in the same line, i'd split it in Window frame position and Window frame size
  • In the WDS we check for multiple windows and report an error, i think we need those output only when doing modification, what do you think?

In any case I can see the code review from this link https://github.com/MonoS/SupMover/pull/18/files/8968b7ce51e5384b9684d1a867d82b3ef87393cb
And looks like this
image

@YourMJK
Copy link
Contributor Author

YourMJK commented Feb 23, 2024

That's probably because I don't know how to use github :)

That's fine, I'm not familiar with code review on GitHub either :)
But I think you need to click on "Review changes" and then "Submit review" for me to see it. Those comments are still pending (see your screenshot).

  • The new --trace doesn't have the check for its option without dash

That was on purpose since it's part of the old syntax and thus doesn't need to be backwards compatible.
But for consistency I can of course add that check as well.

Agreed on the other two points!

main.cpp Outdated Show resolved Hide resolved
main.cpp Outdated Show resolved Hide resolved
main.cpp Outdated Show resolved Hide resolved
@YourMJK YourMJK requested a review from MonoS February 23, 2024 22:47
@YourMJK
Copy link
Contributor Author

YourMJK commented Feb 24, 2024

Should be fixed now

@MonoS MonoS merged commit 1bb2be3 into MonoS:master Feb 25, 2024
2 checks passed
@MonoS
Copy link
Owner

MonoS commented Feb 25, 2024

Merged, thank you :)

@YourMJK YourMJK deleted the trace-option branch February 26, 2024 17:02
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.

2 participants