Skip to content

Feature Addition: Reverse Option #79

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

Merged
merged 3 commits into from
May 6, 2024
Merged

Conversation

djosh34
Copy link
Contributor

@djosh34 djosh34 commented Mar 1, 2024

Hey mlange-42,

Just wanted to drop in a pull request for something you might find a bit hacky, but I found myself needing a reverse option. The approach here is a bit unconventional – instead of overhauling the code, I went for reversing the text_lines and grid. To tackle the issue of characters facing the wrong direction, I flipped them (so a 'left-up' becomes 'left-down', and so on).

It might be more ideal to adjust how the Character struct is accessed with the reverse option in mind, but that would require a significant refactor of the existing code. From what I've tested, it looks like it works pretty well across different character sets. Would love to hear your thoughts on this and if it's something you'd consider merging.

Cheers!

@mlange-42
Copy link
Owner

Hi @djosh34,
thank you for this PR! I am totally ok with this solution, given how simple and effective it is compared to the alternatives.

Will give it a try as soon as I can. Meanwhile, please fix the linter error.

@djosh34
Copy link
Contributor Author

djosh34 commented Mar 6, 2024

@mlange-42 Cargo clippy gave another warning about elided lifetimes. Was it suppose to stay there? In the second commit is still took it away to make clippy happy, but I'm not sure if that is what you want

@mlange-42
Copy link
Owner

mlange-42 commented Mar 6, 2024

It was the failed fmt check that I meant. It did not even proceed to clippy. The lifetime thing is a change to clippy, introduced after the last run of the CI. So Rust has became smarter there. It is great that you fixed it.

@djosh34
Copy link
Contributor Author

djosh34 commented Mar 6, 2024

Yes I forgot to mention that I indeed also did a fmt

@djosh34
Copy link
Contributor Author

djosh34 commented May 5, 2024

Dear @mlange-42,

I have addressed all fmt errors, and GitHub confirms the pull request is ready for merging. I understand you could have a busy schedule, but if possible, could you please test the feature and proceed with the merge? It would be greatly appreciated.

Thank you.

Best regards

@mlange-42
Copy link
Owner

@djosh34 Sorry, this slipped through...

@mlange-42 mlange-42 merged commit f9f3c73 into mlange-42:master May 6, 2024
2 checks passed
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