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

Add custom pages to list output #285

Merged
merged 3 commits into from
Sep 24, 2022
Merged

Add custom pages to list output #285

merged 3 commits into from
Sep 24, 2022

Conversation

Olavhaasie
Copy link
Contributor

Fix #205

I have added custom pages to the output of list by reading from the custom_pages_dir from the config and walking that directory for any pages. This also includes custom pages in the autocompletion.

This is my first contribution, so please let me know if something has to be changed or added 😇

Copy link
Collaborator

@niklasmohrin niklasmohrin left a comment

Choose a reason for hiding this comment

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

Thank you for your submission! I think that you hit the right spot of the codebase with your addition and that the change is useful. I left some comments about small implementation details which I would like to see added, other than that the code looks good. If you feel like it, you can also apply some of my comments to the rest of the function, as they also apply there, but don't feel obliged to do more than you came to do. Finally, I think we should add an integration test to verify that the new feature is working as intended (and to have this check in the future so that we don't break it again). To do so, we already have some setup in tests/lib.rs, you can add a test that checks that a custom page shows up in tldr --list. If you need any help, feel free to reach out :)

src/cache.rs Outdated Show resolved Hide resolved
src/cache.rs Outdated Show resolved Hide resolved
src/cache.rs Outdated Show resolved Hide resolved
src/cache.rs Outdated Show resolved Hide resolved
@Olavhaasie
Copy link
Contributor Author

Thanks for the useful feedback! It really reduced the complexity of the code and I learned something new.
As you said I also applied some changes to the already existing code, similar to the ones you suggested. Also, I added a test case for listing custom pages.

Copy link
Collaborator

@niklasmohrin niklasmohrin left a comment

Choose a reason for hiding this comment

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

@dbrgn ?

@niklasmohrin niklasmohrin requested a review from dbrgn September 14, 2022 18:13
Copy link
Collaborator

@dbrgn dbrgn left a comment

Choose a reason for hiding this comment

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

Looks great! Thanks @Olavhaasie for the contribution and @niklasmohrin for the review!

@dbrgn dbrgn merged commit f4a9411 into tealdeer-rs:main Sep 24, 2022
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Development

Successfully merging this pull request may close these issues.

tldr --list should include customize pages
3 participants