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

feat: refactor --tldr flag #47

Merged
merged 15 commits into from
Nov 21, 2024
Merged

Conversation

hemanialaparthi
Copy link
Collaborator

@hemanialaparthi hemanialaparthi commented Nov 9, 2024

feat: Updated TLDR flag

1. Enhance TLDR Command Examples with Realistic Usage Patterns

This update makes the --tldr output more practical and easier to maintain while following established documentation conventions.

2. List the names of those who contributed to the project.
@hemanialaparthi

3. Link the issue the pull request is meant to fix/resolve.
#39

4. Add all labels that apply. (e.g., documentation, ready-for-review)
bug, enchancement, ready-for-review

5. Describe the contents and goal of the pull request.
This PR improves the --tldr feature by making the command examples more practical and maintainable:

  • Restructure command examples into a dictionary format for better code organization
  • Remove Poetry-specific prefixes for broader applicability
  • Replace abstract placeholders (like ) with concrete, realistic examples

These changes align our documentation style with established conventions like tldr-pages, making the examples more immediately useful for users.

6. Will coverage be maintained/increased?
This feature will not need coverage so coverage will be maintained.

7. What operating systems has this been tested on? How were these tests conducted?
This has been tested on mac, would appreciate testing on linux and windows.

8. Include a code block and/or screenshots displaying the functionality of your feature, if applicable/possible.

The command: poetry run execexam --tldr

The NEW intended output:
image

@hemanialaparthi hemanialaparthi added bug Something isn't working enhancement New feature or request labels Nov 9, 2024
Copy link
Collaborator

@rebekahrudd rebekahrudd left a comment

Choose a reason for hiding this comment

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

I ran this on Linux.

My output was formatted a little differently but it still works and completes the intended job. Here is my output:
image

Something to consider is that the red output that says to make sure I am in a directory with a pyproject.toml file makes me think that I'm not in that directory. It makes me think there's an error when there isn't one.

@hemanialaparthi
Copy link
Collaborator Author

I ran this on Linux.

My output was formatted a little differently but it still works and completes the intended job. Here is my output: image

Something to consider is that the red output that says to make sure I am in a directory with a pyproject.toml file makes me think that I'm not in that directory. It makes me think there's an error when there isn't one.

Hey @rebekahrudd! This is the output of the old --tldr flag. You might want to use the new branch and doing poetry update to get the changes

@PCain02
Copy link
Collaborator

PCain02 commented Nov 11, 2024

I got the output on Windows 10 that looks good. However, I will say I did poetry update and I got the same red output as @rebekahrudd and I agree it does make it a little confusing.

image
image

@@ -39,46 +39,50 @@ def display_tldr(console: Console) -> None:
"[bold red]Please ensure you are in the directory with the pyproject.toml file to run these commands.[/bold red]\n"
Copy link
Collaborator

Choose a reason for hiding this comment

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

Here is the line @rebekahrudd and I are referring to with the red output.

Copy link
Collaborator Author

Choose a reason for hiding this comment

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

I get how this could be confusing! I just removed it, could you please check it out again? Thanks!

Copy link
Collaborator

@hannahb09 hannahb09 left a comment

Choose a reason for hiding this comment

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

I got it to work on linux.
Screenshot from 2024-11-11 18-46-54

@rebekahrudd rebekahrudd removed the request for review from PCain02 November 12, 2024 04:02
@gkapfham
Copy link
Collaborator

Hello @hemanialaparthi will you need to update the pyproject.toml file?

@hemanialaparthi
Copy link
Collaborator Author

Hello @hemanialaparthi will you need to update the pyproject.toml file?

Hi @gkapfham! Yes, I did update the pyproject.toml file with my name as one of the authors and also bumped up the version to 0.3.2

Copy link
Collaborator

@PCain02 PCain02 left a comment

Choose a reason for hiding this comment

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

LGTM!

Copy link
Collaborator

@CalebKendra CalebKendra left a comment

Choose a reason for hiding this comment

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

the command is just execexam not run execexam. Change this string and it looks good!

@hemanialaparthi
Copy link
Collaborator Author

the command is just execexam not run execexam. Change this string and it looks good!

Thanks! I just changed it.

Copy link
Collaborator

@CalebKendra CalebKendra left a comment

Choose a reason for hiding this comment

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

LGTM

@gkapfham
Copy link
Collaborator

Hi @hemanialaparthi I have implemented the feature that removes the extra space. I am going to try to resolve the test that is not passing on NixOS. I will then commit that to this PR and then confirm that everything still passes. If this works then we will need to update the PR #41 so that it has my upgraded version of the test.

@hemanialaparthi
Copy link
Collaborator Author

Hi @hemanialaparthi I have implemented the feature that removes the extra space. I am going to try to resolve the test that is not passing on NixOS. I will then commit that to this PR and then confirm that everything still passes. If this works then we will need to update the PR #41 so that it has my upgraded version of the test.

Awesome! Thank you for letting me know!

@gkapfham
Copy link
Collaborator

Note: learned from @PCain02 that the test failed in the same way on Windows before it was patched.

@gkapfham
Copy link
Collaborator

Hi @PCain02 and @hemanialaparthi I'm nearly finished with work on test cases that will pass. I will make sure that everything works and then merge this PR as soon as my schedule permits. Thanks for your help and hard work!

@hemanialaparthi
Copy link
Collaborator Author

Hi @PCain02 and @hemanialaparthi I'm nearly finished with work on test cases that will pass. I will make sure that everything works and then merge this PR as soon as my schedule permits. Thanks for your help and hard work!

@gkapfham, thank you for working on this!!

@PCain02
Copy link
Collaborator

PCain02 commented Nov 21, 2024

Hi @PCain02 and @hemanialaparthi I'm nearly finished with work on test cases that will pass. I will make sure that everything works and then merge this PR as soon as my schedule permits. Thanks for your help and hard work!

Awesome! Let me know if there are any issues or things I can help with!

@gkapfham
Copy link
Collaborator

Hi @hemanialaparthi please refer to PR #54 for an extension of this PR that takes into account all of your work and then extends it with new tests and repairs to the way in which other tests are run.

@gkapfham gkapfham merged commit e8136ca into GatorEducator:main Nov 21, 2024
3 checks passed
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
bug Something isn't working enhancement New feature or request
Projects
None yet
Development

Successfully merging this pull request may close these issues.

6 participants