-
Notifications
You must be signed in to change notification settings - Fork 0
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
Extract capybara #1
Conversation
4c80678
to
885e5d9
Compare
2291a6d
to
ffa14ee
Compare
ffa14ee
to
2f80e6d
Compare
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Self-review
YARD::Tags::Library.define_tag('Cop Safety Information', :safety) | ||
YARD.parse(Dir[glob].prepend(rspec_cop_path), []) |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
We may need to get this back if we introduce a base class.
rubocop-capybara.gemspec
Outdated
'git@nilsgemeinhardt.de' | ||
] | ||
spec.homepage = 'https://github.com/rubocop/rubocop-capybara' | ||
spec.authors = ['foo'] # TODO |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Speaking about authors, @ydah you seem to have added more than a half of cops.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
If you need an email address, use the following
t.yudai92@gmail.com
[ask] Should we move it to https://github.com/rubocop/rubocop-capybara? |
- Gem is no longer 20MB (sorry!). ([@nevir]) | ||
- `RspecFileName` cop allows for method specs to organized into directories by class and type. ([@nevir]) | ||
|
||
## 1.0.rc1 | ||
|
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Unnecessary line breaks are likely here.
I'd love to move the final result there. With only history related to files currently in the repository, without the noise about all past unrelated RSpec cop changes. I still have one idea left - to let But this PR still stands, as it has all the needed changes, and the resulting file structure and contents are the same. |
# @!method as_is_matcher(node) | ||
def_node_matcher :as_is_matcher, <<-PATTERN | ||
(send | ||
#expectation_set_on_current_path ${:to :to_not :not_to} |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Perhaps, but it seems that there are still remnants of RuboCop RSpec here. I think ${:to :to_not :not_to}
is a peculiar RSpec expression.
But we believe this can be followed up later.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I see those options:
- keep this cop in
rubocop-rspec
, since it's specific to RSpec - keep this cop here, since it's specific to Capybara
- extract to
rubocop-rspec-capybara
orrubocop-capybara-rspec
- keep this cop here, but move it to
Capybara/RSpec
sub-department
What is your take on this?
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
- keep this cop here, since it's specific to Capybara
- keep this cop here, but move it to Capybara/RSpec sub-department
These opinions seem to be good to me.
Would it be good to have a sub-department for each where Expectation is involved 🤔.
There may be unintended side effects if one tries to handle it in one cop. But I haven't found a specific problem yet.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Looks good! Thanks for the great work 🚀
I've figured out a reliable and more or less simple method to keep the history for a limited number of files in the repo: git clone --no-local rubocop-rspec rubocop-capybara-hist
cd rubocop-capybara-hist
git-filter-repo $( (cat ../rubocop-capybara/rename | xargs -I_ echo '--path-rename _ ') )
git-filter-repo $( (cd ../rubocop-capybara && git ls-files) | xargs -I_ echo '--path _ ')
git remote add origin git@github.com:rubocop/rubocop-capybara.git
git push --set-upstream origin master
git checkout -b capybara-only
cd ../rubocop-capybara
cp -r * ../rubocop-capybara-hist
cp -r $( git ls-files | grep '^\.' | grep -v '\.github' ) ../rubocop-capybara-hist
cp -r .github ../rubocop-capybara-hist
cd -
git commit --all --message 'RSpec -> Capybara'
rvm use --create 3.0.4@rubocop-capybara
bundle
rake
git push origin capybara-only
hub pull-request --assign pirj
I intend to:
Does it sound like a plan? |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Great work @pirj ❤️
Just to double check, the removal of FeatureMethods
is intentional, right?
@@ -112,7 +110,7 @@ Layout/LineEndStringConcatenationIndentation: | |||
Style/EmptyHeredoc: | |||
Enabled: true | |||
|
|||
# Enable our own pending cops. | |||
# Enable pending rubocop-rspec cops. |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
The .github/PULL_REQUEST_TEMPLATE.md says “[ ] The cop is configured as Enabled: true in .rubocop.yml.” That should probably be removed, as it only applies to RSpec cops.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Good catch 👍
@bquorning yes, I kept the cop in |
[ask] It looks like the following also needs to be corrected, what do you think? rubocop-capybara/MIT-LICENSE.md Line 3 in 495e175
|
No longer needed, even for history purposes. |
No description provided.