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

Typo #46

Closed
wants to merge 6 commits into from
Closed

Typo #46

wants to merge 6 commits into from

Conversation

someshsingh22
Copy link
Member

@someshsingh22 someshsingh22 commented May 11, 2020

Typo Try Except Removed, excess code removed, random.sample() in place of random.randint

Note : The coverage error is still there from issue #28

@codecov
Copy link

codecov bot commented May 11, 2020

Codecov Report

Merging #46 into master will decrease coverage by 0.20%.
The diff coverage is 88.88%.

Impacted file tree graph

@@            Coverage Diff             @@
##           master      #46      +/-   ##
==========================================
- Coverage   91.32%   91.12%   -0.21%     
==========================================
  Files           6        5       -1     
  Lines         173      124      -49     
==========================================
- Hits          158      113      -45     
+ Misses         15       11       -4     
Impacted Files Coverage Δ
decepticonlp/transforms/perturbations.py 84.72% <88.88%> (-1.00%) ⬇️
decepticonlp/transforms/paraphrase.py

Continue to review full report at Codecov.

Legend - Click here to learn more
Δ = absolute <relative> (impact), ø = not affected, ? = missing data
Powered by Codecov. Last update e3c6dde...b817d76. Read the comment docs.

@someshsingh22 someshsingh22 linked an issue May 11, 2020 that may be closed by this pull request
@someshsingh22 someshsingh22 marked this pull request as ready for review May 11, 2020 20:36
word[i] = random.choice(keys_in_proximity[c])
if cap:
# convert to upper if in upper
word[i] = word[i].upper()
Copy link
Member

Choose a reason for hiding this comment

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

Add test case to cover this also.
That should resolve the coverage failure?

Copy link
Member Author

Choose a reason for hiding this comment

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

The test case is already there, does code coverage count the number of test cases for logic ?

Copy link
Member Author

Choose a reason for hiding this comment

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

A simpler resolve would be to extend the dictionary to capital letters.

Pros - Defining non alphabetical typos as well, the current implementation and this PR to assume that typos consider letters only, however, I can also put a number in between. So we can define different typos for capital letters

Cons - Will clutter the code.

Should I save this dictionary as a son rather ? This way in future if we wish to add any changes we don't have to mess with the code

@someshsingh22 someshsingh22 linked an issue May 12, 2020 that may be closed by this pull request
@sonarcloud
Copy link

sonarcloud bot commented May 12, 2020

Kudos, SonarCloud Quality Gate passed!

Bug A 0 Bugs
Vulnerability A 0 Vulnerabilities (and Security Hotspot 1 Security Hotspot to review)
Code Smell A 0 Code Smells

No Coverage information No Coverage information
0.0% 0.0% Duplication

@someshsingh22 someshsingh22 deleted the Typo branch May 12, 2020 11:39
@someshsingh22
Copy link
Member Author

This Pull request will be completed in #52

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.

Adding more characters to the dictionary in the typo function Typo Try Catch Exceptions
2 participants