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

Add formatters #24

Merged
merged 3 commits into from Apr 8, 2015
Merged

Add formatters #24

merged 3 commits into from Apr 8, 2015

Conversation

ghost
Copy link

@ghost ghost commented Feb 25, 2015

This is a continuation of #23 which tries to fix the issue #12 .

Currently two formatters classes are available, the default one IndentWithTabsFormatterand a new VerticalHangingFormatter.

All tests pass, however I don't know anything about mock, and I don't know if I did it right, and I can't figure out how to add tests for new config and argument formatter.

Here's the TODO from #23 :

  • add --formatter option in the script
  • refactor formatted() tests to test_formatters.py
  • add new tests for new formatter
  • add some docs with examples of difference in formatters
  • update README

I think everything is OK,


Parameters
----------
statement : ImportStatement instance
Copy link
Owner

Choose a reason for hiding this comment

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

nitpik - this is numpy doc - on first line you have var name and its type. on next then you add description. so instance is not necessary

Copy link
Author

Choose a reason for hiding this comment

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

Should I just remove the word instance?

- Added a formatter file to store formatter classes
- Ignore flake8 E731,W503 due to recent update of pep8
- Added a new VerticalHangingFormatter
- Added a separate file to test formatters
- Add config & argument and check existence of formatter
- Fix tests for new formatter config/arg
- Updated README to explain usage of formatters
- Added pypy3 interpreter to tests
@coveralls
Copy link

Coverage Status

Changes Unknown when pulling e2188f6 on benjaminabel:formatters into * on miki725:formatters*.

string += '{})'.format(self.file_artifacts.get('sep', '\n'))

return string
def formatted(self, formatter='IndentWithTabsFormatter'):
Copy link
Owner

Choose a reason for hiding this comment

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

I would rather pass here actual class so no import is necessary. Since formatters could be pretty generic, we might want to allow people to use custom formatters even if they are not in core importanize...

@ghost
Copy link
Author

ghost commented Feb 28, 2015

@miki725 When browsing django code, I've seen that Django already use a tool to sort imports see this commit. It's isort, and can use different format options see README. It seems really powerfull with numerous config options however, I think the way you treat comments is much more precise. Do you know this project?

@miki725
Copy link
Owner

miki725 commented Feb 28, 2015

@benjaminabel haha. good questions. I needed a utility for sorting imports at work which is why I started importanize. When I was done, only then I found out about isort. I really like isort for its flexibility and availability of many options. However I like the way I handle multiple import groups better since in importanize you explicitly specify the import groups and what packages go into which group. For example you can have as many import groups as like you which is not possible in isort (as of last time I checked) and this was one of the requirements for me to use importanize at work.

@ghost
Copy link
Author

ghost commented Mar 1, 2015

I tried to implement your changes however I can't figure out how to pass the formatter class catched from the main.py file to the ImportStatement class in statements file. @minrk thanks for your help and your reviews. I don't think I can go further on the issue. Do you want that I close it or you may want to finish it yourself?

@miki725
Copy link
Owner

miki725 commented Mar 1, 2015

No problem. I can finish this. Thanks for contributing!
Before I merge can you add your name in authors file and then when Ill have a chance, Ill wrap this up.

@ghost
Copy link
Author

ghost commented Mar 2, 2015

This is done. Thanks for your help @miki725

@miki725
Copy link
Owner

miki725 commented Mar 2, 2015

ok. great. thanks @benjaminabel for your contribution. Ill leave the PR open until Ill have a chance to finish this up in case you will want to push more changes.

@ghost
Copy link
Author

ghost commented Mar 2, 2015

I don't think I got the technical capabilities to finish this. I need to learn more!

@miki725
Copy link
Owner

miki725 commented Mar 2, 2015

I think all of us need to learn more!

miki725 added a commit that referenced this pull request Apr 8, 2015
@miki725 miki725 merged commit 25361ad into miki725:formatters Apr 8, 2015
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.

2 participants