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: Modify search/text_search.cpp #1662

Merged
merged 16 commits into from
Oct 14, 2021

Conversation

anuran-roy
Copy link
Contributor

@anuran-roy anuran-roy commented Oct 3, 2021

Description of Change

Modified text_search.cpp to include case-insensitive search. Also added test function to check for custom function called lower()

Checklist

  • Added description of change
  • Added file name matches File name guidelines
  • Added tests and example, test must pass
  • Added documentation so that the program is self-explanatory and educational - Doxygen guidelines
  • Relevant documentation/comments is changed or added
  • PR title follows semantic commit guidelines
  • Search previous suggestions before making a new one, as yours may be a duplicate.
  • I acknowledge that all my contributions will be made under the project's license.

Notes:

@anuran-roy
Copy link
Contributor Author

@Panquesito7 I made a new PR as you suggested. For the blockchain code, I'll soon add it after sufficient changes. Can you please review this? 😃

Copy link
Member

@Panquesito7 Panquesito7 left a comment

Choose a reason for hiding this comment

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

Great work! 😄

search/text_search.cpp Show resolved Hide resolved
search/text_search.cpp Outdated Show resolved Hide resolved
search/text_search.cpp Outdated Show resolved Hide resolved
@Panquesito7 Panquesito7 added enhancement New feature or request requested changes changes required labels Oct 3, 2021
@Panquesito7 Panquesito7 changed the title Modify search/text_search.cpp feat: Modify search/text_search.cpp Oct 3, 2021
search/text_search.cpp Outdated Show resolved Hide resolved
@@ -12,7 +13,31 @@

/** Main function
*/

std::string lower(std::string word) { // convert a C++ string to lowercase
Copy link
Member

Choose a reason for hiding this comment

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

Still missing proper documentation (check the typical structure of a program as a reference).

@anuran-roy
Copy link
Contributor Author

@siriak can you please see it? 😅 I guess @Panquesito7 is a bit occupied with some important work...

@siriak
Copy link
Member

siriak commented Oct 5, 2021

@Panquesito7 needs to review it before merging anyway

Copy link
Member

@Panquesito7 Panquesito7 left a comment

Choose a reason for hiding this comment

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

Almost there! 😄

search/text_search.cpp Outdated Show resolved Hide resolved
search/text_search.cpp Show resolved Hide resolved
search/text_search.cpp Outdated Show resolved Hide resolved
@anuran-roy
Copy link
Contributor Author

@Panquesito7 I'm done with the required changes 😃

Comment on lines 40 to 41
/** Main function
*/
Copy link
Member

@Panquesito7 Panquesito7 Oct 6, 2021

Choose a reason for hiding this comment

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

It's just to accept this suggestion, and you're done! 😄

Suggested change
/** Main function
*/
/**
* @brief Main function
* @returns 0 on exit
*/

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Yeah sure! 😄

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Done 😃

Panquesito7
Panquesito7 previously approved these changes Oct 6, 2021
Copy link
Member

@Panquesito7 Panquesito7 left a comment

Choose a reason for hiding this comment

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

Amazing work! 🚀 Thank you for your contribution! We hope you keep contributing. 😄👍🎉

@Panquesito7 Panquesito7 added approved Approved; waiting for merge Improvement improvement in previously written codes and removed enhancement New feature or request requested changes changes required labels Oct 6, 2021
@anuran-roy
Copy link
Contributor Author

Thank you so much for helping me out throughout the process @Panquesito7 😄 I'll contribute more!

.gitignore Outdated
@@ -34,4 +34,4 @@ a.out
*.out
*.app

build/
build/
Copy link
Member

Choose a reason for hiding this comment

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

undo this change

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Isn't this same? I don't get what's different here 😅

.vscode/settings.json Outdated Show resolved Hide resolved
@anuran-roy
Copy link
Contributor Author

@Panquesito7 @ayaankhan98 Uhh... Anything that I should do to make this branch good to merge?

Panquesito7
Panquesito7 previously approved these changes Oct 14, 2021
@Panquesito7 Panquesito7 merged commit 85721be into TheAlgorithms:master Oct 14, 2021
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
approved Approved; waiting for merge Improvement improvement in previously written codes
Projects
None yet
Development

Successfully merging this pull request may close these issues.

4 participants