Skip to content

Refactor RSpec for AnnotateModels (5) - structuralize test cases #741

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

Conversation

nard-tech
Copy link
Collaborator

The test cases of AnnotateModels.get_schema_info was separated into two clauses.
So I merged them into one clause, and refactor test cases.

Copy link
Collaborator

@drwl drwl left a comment

Choose a reason for hiding this comment

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

Had a question about putting subject under a context. Some of the commits are very big diffs and hard to review:

e9784b0
d8aaee9
6dfa47f

Could you break them into smaller ones?

let :header do
'Schema Info'
context 'when option is not present' do
subject do
Copy link
Collaborator

Choose a reason for hiding this comment

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

Any reason why is subject under context instead of describe?

Copy link
Collaborator Author

Choose a reason for hiding this comment

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

@drwl Thanks, I'd like to split large commits into smaller ones.

I use context and describe depending on following link.
https://lmws.net/describe-vs-context-in-rspec

Are you getting at that subject under context is strange and subject should be under describe?

Copy link
Collaborator

Choose a reason for hiding this comment

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

Yep, I've usually seen subject be under describe instead of context but there doesn't seem to be anything on the style guide against it.

https://github.com/rubocop-hq/rspec-style-guide#subject-naming-in-context

Has subject under multiple contexts so seems to be fine.

Copy link
Collaborator Author

Choose a reason for hiding this comment

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

@drwl All right. I'll use your reply as a reference in the next PR. Thank you.

@nard-tech nard-tech closed this Feb 2, 2020
drwl pushed a commit that referenced this pull request Feb 13, 2020
The test cases of AnnotateModels.get_schema_info was separated into two sections.
So I merged them into one, and refactor test cases.

This PR is new version of #741.
vfonic pushed a commit to vfonic/annotate_models that referenced this pull request May 8, 2020
The test cases of AnnotateModels.get_schema_info was separated into two sections.
So I merged them into one, and refactor test cases.

This PR is new version of ctran#741.
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