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

Applied case sensitivity settings to KeywordsHelper #735

Open
wants to merge 10 commits into
base: master
Choose a base branch
from

Conversation

abbasc52
Copy link
Contributor

Extended case sensitivity settings to KeywordHelper

@abbasc52
Copy link
Contributor Author

@StefH, I am unable to find the right place to add test cases for this, could you guide me?

@StefH StefH changed the title Applied case sensitivity settings to KeywordHelper Applied case sensitivity settings to KeywordsHelper Aug 16, 2023
@StefH
Copy link
Collaborator

StefH commented Aug 16, 2023

@abbasc52

  1. Changing this could break some existing usage. So in case this functionality is added, a new setting KeywordsAreCaseSensitive is needed.

  2. Add a new test here \Parser\KeywordsHelperTests.cs

@abbasc52
Copy link
Contributor Author

@StefH Addressed your comment and added test cases, please review again

@abbasc52
Copy link
Contributor Author

@StefH when can we merge/release this PR?

@StefH
Copy link
Collaborator

StefH commented Oct 21, 2023

@StefH when can we merge/release this PR?

I'll take a look.

@StefH
Copy link
Collaborator

StefH commented Oct 29, 2023

because of PR #755, this PR needs to be updated....

@abbasc52
Copy link
Contributor Author

abbasc52 commented Nov 25, 2023

@StefH I have updated the PR. Although looking at your PR, you seem to have fixed case issue with custom types. This PR will only help people make Context keywords case sensitive via config . I will leave it upto you if you want to complete this PR.

@abbasc52
Copy link
Contributor Author

My original goal of this PR was to make predefined and custom types case sensitive :)

@StefH
Copy link
Collaborator

StefH commented Nov 28, 2023

@abbasc52
See my review comments.

@abbasc52 abbasc52 requested a review from StefH January 13, 2024 08:23
@abbasc52
Copy link
Contributor Author

@StefH fixed the review comments

public void TryGetValue_WithCaseSensitiveSettings_ReturnsResultAsExpected(string name, bool areKeywordsCaseSensitive,bool expected)
{
// Arrange
var keywordsHelper = this.CreateKeywordsHelper(new ParsingConfig { AreKeywordsCaseSensitive = areKeywordsCaseSensitive });
Copy link
Collaborator

Choose a reason for hiding this comment

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

this i not needed

[InlineData("TestClass", true, true)]
[InlineData("testClass", true, false)]
[InlineData("nonExisting", true ,false)]
public void TryGetValue_WithCaseSensitiveSettings_ReturnsResultAsExpected(string name, bool areKeywordsCaseSensitive,bool expected)
Copy link
Collaborator

Choose a reason for hiding this comment

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

Do a format file to align the source code (adding some spaces)

@StefH
Copy link
Collaborator

StefH commented Jan 23, 2024

@abbasc52
See my review comments.

1 similar comment
@StefH
Copy link
Collaborator

StefH commented Feb 7, 2024

@abbasc52
See my review comments.

@StefH
Copy link
Collaborator

StefH commented Feb 29, 2024

Hello @abbasc52, can you please take a look at my review comments?

1 similar comment
@StefH
Copy link
Collaborator

StefH commented Apr 6, 2024

Hello @abbasc52, can you please take a look at my review comments?

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.

None yet

2 participants