Skip to content

Conversation

@iggi42
Copy link

@iggi42 iggi42 commented Oct 28, 2025

I learned that strtrim should only remove characters of the given set at the end and start of the input string.
This tester lacked a test to catch that misunderstanding, so here have one.

Copy link
Author

@iggi42 iggi42 left a comment

Choose a reason for hiding this comment

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

woops, that was meant to go another branch.

@Tripouille Tripouille requested a review from Copilot December 4, 2025 18:35
Copy link

Copilot AI left a comment

Choose a reason for hiding this comment

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

Pull request overview

This PR adds a test case for ft_strtrim to verify it only removes characters from the given set at the start and end of the input string, not from the middle. Additionally, it fixes file permission issues in file creation calls and updates build configuration paths and compiler flags.

  • Added a new test case for ft_strtrim to ensure characters are only trimmed from string boundaries
  • Fixed missing file permission mode in open() calls across multiple test files
  • Updated LIBFT_PATH to point to a subdirectory and added compiler flag to suppress unused result warnings

Reviewed changes

Copilot reviewed 5 out of 6 changed files in this pull request and generated no comments.

Show a summary per file
File Description
tests/ft_strtrim_test.cpp Added test case verifying trim-only-at-boundaries behavior
tests/ft_putstr_fd_test.cpp Added missing file permissions (0600) to open() call
tests/ft_putendl_fd_test.cpp Added missing file permissions (0600) to open() call
tests/ft_putchar_fd_test.cpp Added missing file permissions (0600) to open() call
Makefile Updated library path to subdirectory and added compiler warning suppression flag

💡 Add Copilot custom instructions for smarter, more guided reviews. Learn how to get started.

@Tripouille
Copy link
Owner

Could you please update this PR by including only the scenario you want to add as changes? Thanks.

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