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

Refactor RSpec for AnnotateModels #718

Conversation

nard-tech
Copy link
Collaborator

@nard-tech nard-tech commented Jan 12, 2020

I refactored and structuralized RSpec test cases of AnnotateModels for readability and scalability because it was too complex to read.

@nard-tech nard-tech force-pushed the feature/refactor_annotate_models/rspec/clean branch from a0f37e5 to 64d38f4 Compare January 12, 2020 20:36
@drwl
Copy link
Collaborator

drwl commented Jan 15, 2020

🙌 thanks for doing this. Github has a hard time showing the changes so I'll carve out some time to review this.

@drwl drwl self-assigned this Jan 15, 2020
@nard-tech
Copy link
Collaborator Author

@drwl Thank you. I am waiting you to review this. After I made this PR, I notice that I can unify describe clause, so I added commits. (All test cases are passed)
Please review including these commits.

@nard-tech nard-tech force-pushed the feature/refactor_annotate_models/rspec/clean branch from dfdbe8a to 16d9738 Compare January 15, 2020 21:01
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.

First off I want to say thank you for taking the time to refactor the specs. They are in pretty bad shape and after this refactor gets in they will be much better.

I went commit by commit to try and review but found these commits to be too big for me to review:

28e5e84
b7cac0a
7b2697e

Would it be possible for you to split these ones up?

spec/lib/annotate/annotate_models_spec.rb Show resolved Hide resolved
spec/lib/annotate/annotate_models_spec.rb Show resolved Hide resolved
spec/lib/annotate/annotate_models_spec.rb Show resolved Hide resolved
spec/lib/annotate/annotate_models_spec.rb Show resolved Hide resolved
spec/lib/annotate/annotate_models_spec.rb Show resolved Hide resolved
spec/lib/annotate/annotate_models_spec.rb Show resolved Hide resolved
Gemfile Show resolved Hide resolved
@nard-tech
Copy link
Collaborator Author

nard-tech commented Jan 17, 2020

@drwl Thank you for your replying and pointing my mistakes out.
I already fixed commits in my local environment in accordance with your indication.

As you requested

I went commit by commit to try and review but found these commits to be too big for me to review:
28e5e84
b7cac0a
7b2697e
Would it be possible for you to split these ones up?

so I'd like to

  • close this PR
  • make another new PR including fixes, without these 3 commits
  • after the new PR is merged, make the other PR spliting up these 3 commits

May I continue to contribute in this way?

@drwl
Copy link
Collaborator

drwl commented Jan 18, 2020

May I continue to contribute in this way?

Sure that sounds good to me. Again thanks for all this work, it is really appreciated 👍

@nard-tech
Copy link
Collaborator Author

@drwl All right, then I will close this commit. Thank you.

@nard-tech nard-tech closed this Jan 19, 2020
@nard-tech nard-tech deleted the feature/refactor_annotate_models/rspec/clean branch January 19, 2020 12:22
drwl pushed a commit that referenced this pull request Jan 19, 2020
I refactored and structuralized RSpec test cases of AnnotateModels for readability and scalability because it was too complex to read.

cf. #718 
In this PR, I refactored test cases of some methods in `AnnotateModels`.
I will refactor test cases of other methods in another PR.
vfonic pushed a commit to vfonic/annotate_models that referenced this pull request May 8, 2020
I refactored and structuralized RSpec test cases of AnnotateModels for readability and scalability because it was too complex to read.

cf. ctran#718 
In this PR, I refactored test cases of some methods in `AnnotateModels`.
I will refactor test cases of other methods in another PR.
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