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

Support --stdin-display-name and the mixed style of files and stdin #278

Merged
merged 9 commits into from
Jun 29, 2018

Conversation

Kuniwak
Copy link
Member

@Kuniwak Kuniwak commented Jun 20, 2018

Support --stdin-display-name and mixed style linting such as vint a.vim - b.vim (in commented at #240).

@Kuniwak Kuniwak self-assigned this Jun 20, 2018
@Kuniwak Kuniwak mentioned this pull request Jun 20, 2018
README.rst Outdated
@@ -122,6 +122,40 @@ You can configure linting severity, max errors, ... as following:

$ vint --color --style ~/.vimrc

And you can see all available options by `--help` as following:
Copy link
Member

Choose a reason for hiding this comment

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

Better:

You can see all available options by using --help:

Copy link
Member Author

Choose a reason for hiding this comment

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

Fixed at 37a7e49.

README.rst Outdated
--enable-neovim enable Neovim syntax
-f FORMAT, --format FORMAT
set output format
--stdin-alt-path STDIN_ALT_PATH
Copy link
Member

Choose a reason for hiding this comment

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

I do not find that name good.
What about --stdin-display-name? (used by flake8)

Copy link
Member Author

Choose a reason for hiding this comment

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

I feel --stdin-display-name is a good name. Fixed at 7b718a5.

@Kuniwak Kuniwak changed the title Support --stdin-alt-name and the mix style of files and stdin Support --stdin-display-name and the mix style of files and stdin Jun 21, 2018
@Kuniwak Kuniwak changed the title Support --stdin-display-name and the mix style of files and stdin Support --stdin-display-name and the mixed style of files and stdin Jun 21, 2018
Copy link
Member

@blueyed blueyed left a comment

Choose a reason for hiding this comment

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

I feel uneasy about the many changes.

Why is LintTargetFile required?

assert actual_violation['level'] == expected_violation['level']
self.assertEqual(actual_violation['name'], expected_violation['name'])
self.assertEqual(actual_violation['position'], expected_violation['position'])
self.assertEqual(actual_violation['level'], expected_violation['level'])
Copy link
Member

Choose a reason for hiding this comment

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

Why?
Aren't we using pytest here?

Copy link
Member Author

@Kuniwak Kuniwak Jun 22, 2018

Choose a reason for hiding this comment

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

The reason why I remove py.test specific code is the learning cost and the debugging cost. I felt that keep time for implementation shorter was important because I had only few moments to implement it.

In this case, I forget sometimes how to use py.test, so the tests got failures by my mistakes. But understanding what the error messages meant and how to fix it were not easy to me. I think the reason why the error message is hard is that py.test using some magics to implement the parameterized testing feature.

But I do not mean py.test is unuseful for all situation. Only this case, I determined that py.test is unnecessary for me. Feel free to use py.test if using py.test is quick way to acheve what you want.

@Kuniwak
Copy link
Member Author

Kuniwak commented Jun 22, 2018

I feel uneasy about the many changes.

I'm sorry about that. I was in hurry, so I could not separate changes.

Why is LintTargetFile required?

The reason is DRY. I found the following smells:

There are 2 differences for each pair. The first one is what a file path is used. The second one is how to get the file content. We can unify the methods if we implement a new abstraction that encapsulate what a file path is holding and how to get the file content. This abstraction is AbstractLintTarget.

Additionally, I implemented CachedLintTarget to remove this TODO comment. ProhibitMissingScriptEncoding did read files or stdin again, so we can improve the performance by cacheing the content.

@Kuniwak Kuniwak mentioned this pull request Jun 25, 2018
@Kuniwak Kuniwak merged commit 3a2729e into master Jun 29, 2018
@Kuniwak Kuniwak deleted the stdin-improvement branch June 29, 2018 03:35
@Kuniwak Kuniwak mentioned this pull request Jun 29, 2018
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