Skip to content

Improved repr #27

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

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

Improved repr #27

wants to merge 35 commits into from

Conversation

orgh0
Copy link
Collaborator

@orgh0 orgh0 commented Jul 8, 2018

@sushain97 Could you review this ?

@coveralls
Copy link

coveralls commented Jul 8, 2018

Pull Request Test Coverage Report for Build 176

  • 42 of 44 (95.45%) changed or added relevant lines in 5 files are covered.
  • 1 unchanged line in 1 file lost coverage.
  • Overall coverage increased (+0.7%) to 81.135%

Changes Missing Coverage Covered Lines Changed/Added Lines %
apertium/init.py 6 8 75.0%
Files with Coverage Reduction New Missed Lines %
apertium/generation/init.py 1 96.67%
Totals Coverage Status
Change from base Build 159: 0.7%
Covered Lines: 615
Relevant Lines: 758

💛 - Coveralls

Copy link
Member

@sushain97 sushain97 left a comment

Choose a reason for hiding this comment

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

Looks like you're almost there. Do you mind adding a few tests?

README.md Outdated
@@ -59,9 +59,22 @@ In [2]: apertium.append_pair_path('..')

### Translation
Performing Translations
<<<<<<< HEAD
Copy link
Member

Choose a reason for hiding this comment

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

Merge conflict artifacts?

Copy link
Collaborator Author

Choose a reason for hiding this comment

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

@sushain97 No worries, I'll add tests :)

@@ -68,6 +75,14 @@ def test_uninstalled_mode(self):
with self.assertRaises(apertium.ModeNotInstalled):
apertium.generate('spa', 'cat<n><pl>')

def test_repr(self):
generator = apertium.Generator('eng')
self.assertEqual(repr(generator), 'Generator(lang=eng)')
Copy link
Member

Choose a reason for hiding this comment

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

These need single quotes. Generator(lang=eng) itself would error. Similar concern with the others.

Copy link
Collaborator Author

Choose a reason for hiding this comment

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

@sushain97 Just to check if I understand this properly,
The issue is that 'Generator(lang=eng)' is being returned by the function as a string?
But wasn't that the point of repr, it returns a printable representation of the object

@sushain97
Copy link
Member

sushain97 commented Jul 10, 2018 via email

@orgh0
Copy link
Collaborator Author

orgh0 commented Jul 11, 2018

@sushain97 But why? I don't quite understand what the problem with the current repr and str function is

Sorry for the inconvenience 😅

@orgh0
Copy link
Collaborator Author

orgh0 commented Jul 11, 2018

@sushain97 I've changed the code according to my understanding, please review and let me know if I have it right.

@sushain97
Copy link
Member

But why? I don't quite understand what the problem with the current repr and str function is

The issue is that Generator(lang=eng) will syntax error if I put it into a REPL.

@sushain97
Copy link
Member

sushain97 commented Jul 11, 2018

The str versions aren't meant to be runnable so they don't need (or should have) the quotes.

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.

3 participants