Skip to content

New: vue/script-indent rule (fixes #118) #309

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

Merged
merged 12 commits into from
Jan 7, 2018
Merged

Conversation

mysticatea
Copy link
Member

Fixes #118.

This PR adds vue/script-indent rule.
The vue/script-indent rule is sharing all logic with vue/html-indent rule. The shared logic is at lib/utils/indent-common.js.

Also, this PR refactored the tests of vue/html-indent rule. New tests load files from tests/fixtures/html-indent/ then use it as valid cases. And it makes invalid tests from the valid tests automatically -- removing all indentation and checking warning/fixing for correct indentation.
Similarly, the tests of vue/script-indent rule are in tests/fixtures/script-indent/.

@mysticatea
Copy link
Member Author

mysticatea commented Dec 31, 2017

I have imported alignAttributesVertically from #301. This PR moves the implementation of indent rules to lib/utils/indent-common.js, so it couldn't merge simply.

@mysticatea mysticatea added this to the v4.1.0 milestone Dec 31, 2017
@michalsnik
Copy link
Member

Please resolve conflicts in your spare time :) I'll try to review it in the next few days

# Conflicts:
#	lib/rules/html-indent.js
#	tests/lib/rules/html-indent.js
@michalsnik michalsnik modified the milestones: v4.1.0, v4.2.0 Jan 6, 2018
# Conflicts:
#	.eslintignore
#	README.md
const baseObj = JSON.parse(/^<!--(.+?)-->/.exec(code0)[1])
return Object.assign(baseObj, { code, filename })
})
const invalid = valid
Copy link
Member

Choose a reason for hiding this comment

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

It's hard to read and know which fixture is good, or not. You're detecting them here, but I think it would be muuuch easier to just add another comment or put valid fixtures in valid directory and so on. Then you wouldn't need to do any confusing magic here. Or am I'm missing something? (It's very possible :D)

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'll add comments about what this does.

This addresses the code in fixtures as valid.
And this creates an invalid case from every valid case -- it removes all indentation and checks whether the rule restores all indentation or not.

Copy link
Member Author

Choose a reason for hiding this comment

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

So if there are ignored lines, we can't use fixtures. And it's the reason that some tests exist on outside of fixtures.

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 added comments.

To realize invalid sub-directory, it needs a DSL to specify expected indentation for each line. Indeed it would be useful, but I avoided it for my time.

Copy link
Member

Choose a reason for hiding this comment

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

Fine, now it's much easier to understand. Thank you for explanation and comments :)

@michalsnik michalsnik merged commit 784925b into master Jan 7, 2018
@michalsnik michalsnik deleted the script-indent/new branch January 7, 2018 16:05
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Projects
None yet
2 participants