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

Extractor #57

Merged
merged 6 commits into from
May 13, 2020
Merged

Extractor #57

merged 6 commits into from
May 13, 2020

Conversation

someshsingh22
Copy link
Member

@someshsingh22 someshsingh22 commented May 13, 2020

Commit 1 - Basic Random Extractor Module
Commit 2 - Basic OOP Random Extractor Module
Commit 3 - Unit Testing
Commit 4 - Assertion Errors, Minor Fixes from Travis Log
Commit 5 - Pragma added
Commit 6 - .coveragerc Added
Note - Would Recommend Not to squash, can serve as "How To" for OOP implementations

@someshsingh22 someshsingh22 linked an issue May 13, 2020 that may be closed by this pull request
@someshsingh22 someshsingh22 force-pushed the Extractor branch 2 times, most recently from 0366315 to 8511115 Compare May 13, 2020 19:09
@someshsingh22 someshsingh22 marked this pull request as ready for review May 13, 2020 19:11
@codecov
Copy link

codecov bot commented May 13, 2020

Codecov Report

Merging #57 into master will increase coverage by 1.06%.
The diff coverage is 100.00%.

Impacted file tree graph

@@            Coverage Diff             @@
##           master      #57      +/-   ##
==========================================
+ Coverage   92.12%   93.18%   +1.06%     
==========================================
  Files           6        7       +1     
  Lines         165      176      +11     
==========================================
+ Hits          152      164      +12     
+ Misses         13       12       -1     
Impacted Files Coverage Δ
decepticonlp/transforms/perturbations.py 87.83% <ø> (+0.99%) ⬆️
decepticonlp/extractor/basic.py 100.00% <100.00%> (ø)

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 ea1718f...515d8f3. Read the comment docs.

@someshsingh22 someshsingh22 marked this pull request as draft May 13, 2020 19:21
@someshsingh22 someshsingh22 marked this pull request as ready for review May 13, 2020 19:39
@someshsingh22
Copy link
Member Author

someshsingh22 commented May 13, 2020

@rohts-patil, @Sharad24 Can you tell me how is this different from the OOP - implementation of Perturbations, its stuck at the code-coverage for the abstract class

@someshsingh22 someshsingh22 added the help wanted Extra attention is needed label May 13, 2020
@@ -0,0 +1,30 @@
_author_ = "Somesh Singh"
Copy link
Member

Choose a reason for hiding this comment

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

This too

Copy link
Member Author

@someshsingh22 someshsingh22 May 13, 2020

Choose a reason for hiding this comment

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

Still there, codecov shows errors at the abstract class methods idk why

Copy link
Member

Choose a reason for hiding this comment

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

What?

assert random_extractor.extract(words, top_k=2) == expected_result


def random_empty_extract():
Copy link
Member

@Sharad24 Sharad24 May 13, 2020

Choose a reason for hiding this comment

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

pytest can only identify functions/classes starting with test/Test, etc
Rename this to test_random_empty_extract and it should work.

Copy link
Member

Choose a reason for hiding this comment

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

This is the issue. @someshsingh22

Copy link
Member Author

Choose a reason for hiding this comment

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

Ok, that makes sense, I thought it's true only for filenames

Copy link
Member Author

Choose a reason for hiding this comment

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

Thanks !

"""

@abc.abstractmethod
def extract(self, words: list, **kwargs):
Copy link
Member

Choose a reason for hiding this comment

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

nit
Missing argument docstrings?

Copy link
Member

Choose a reason for hiding this comment

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

Also since this function should never be tested, you should add pragma: no cover in front of it. pytest ignores its count in code coverage.

def extract(self, words: list, **kwargs): #pragma: no cover

@Sharad24
Copy link
Member

Sharad24 commented May 13, 2020

Any reason to force push btw?

@someshsingh22
Copy link
Member Author

Reduces unnecessary commits, I wanted this to be a reference PR, for whenever you want to implement a new OOP Module

decepticonlp/extractor/basic.py Outdated Show resolved Hide resolved
@someshsingh22
Copy link
Member Author

I have added pragma in this PR, any where else ?

@Sharad24
Copy link
Member

Only functions with a decorator of an abstract method need to have pragma no cover.

The other functions you've labelled as pragma no cover do get called when testing

tests/test_extractor_basic.py Outdated Show resolved Hide resolved
tests/test_extractor_basic.py Outdated Show resolved Hide resolved
@someshsingh22 someshsingh22 marked this pull request as ready for review May 13, 2020 21:41
Comment on lines 33 to 35
:params:
:words: - List of Words
:top_k: - Number of words to be extracted
Copy link
Member

Choose a reason for hiding this comment

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

This is not the corrent syntax for docstrings, no?

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 class used to apply random word extractor.

    Methods
    -------
    extract(self, words: list, top_k = 1, **kwargs)
        - extracts top_k random words and returns their index.
    
    Parameters
    --------
    words - List of words.
    top_k - Number of words to be extracted
"""

Changed to this

Copy link
Member

Choose a reason for hiding this comment

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

I think its ok for now

@Sharad24
Copy link
Member

You also need to change the pytest command in travis.

Add --cov-config=.coveragerc to it

.travis.yml Outdated
@@ -15,6 +15,6 @@ install:
script:
- black --check decepticonlp
- black --check tests
- pytest --cov='./decepticonlp/'
- pytest --cov-config=.coveragerc
Copy link
Member

@Sharad24 Sharad24 May 13, 2020

Choose a reason for hiding this comment

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

The command should be pytest --cov='./decepticonlp/' --cov-config=.coveragerc

@sonarcloud
Copy link

sonarcloud bot commented May 13, 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

@Sharad24
Copy link
Member

:)

@Sharad24
Copy link
Member

Sharad24 commented May 13, 2020 via email

@someshsingh22 someshsingh22 merged commit f8c57e1 into SforAiDl:master May 13, 2020
@someshsingh22 someshsingh22 deleted the Extractor branch May 13, 2020 23:57
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
help wanted Extra attention is needed
Projects
None yet
3 participants