Adds support to print info for multiple files. #83
Conversation
mateusdeap
requested review from
ashwinisukale,
KostiantynPopovych,
arielj and
etagwerker
and removed request for
a team and
KostiantynPopovych
| opts.on("--path PATH_OR_FILE_GLOB", "Path to deprecation shitlist or glob for multiple files") do |path| | ||
| options[:path] = path | ||
| end |
There was a problem hiding this comment.
I think we should put the use of this option in Readme as well.
| deprecation_warnings = file_paths.map { |path| extract_deprecations(path, pattern) } | ||
| .inject(:merge) |
There was a problem hiding this comment.
the inject(:merge) logic does not work well with more than 2 files.
I did this:
- created a folder
dep_testand added 3 filesdep1.json,dep2.json, anddep3.json.
dep1.json
{
"./spec/controllers/contacts_controller_spec.rb": [
"DEPRECATION WARNING: dep1",
"DEPRECATION WARNING: dep1"
],
"./spec/controllers/search_controller_spec.rb": [
"DEPRECATION WARNING: dep2",
"DEPRECATION WARNING: dep2",
"DEPRECATION WARNING: dep2"
]
}
dep2.json
{
"./spec/controllers/search_controller_spec.rb": [
"DEPRECATION WARNING: dep3",
"DEPRECATION WARNING: dep3"
]
}
dep3.json
{
"./spec/controllers/contacts_controller_spec.rb": [
"DEPRECATION WARNING: dep4"
],
"./spec/controllers/search_controller_spec.rb": [
"DEPRECATION WARNING: dep3",
"DEPRECATION WARNING: dep2",
"DEPRECATION WARNING: dep2"
]
}
Now I run this command
exe/deprecations info --path "/home/rishi/projects/next_rails/dep_test/*"
I get this output
Ten most common deprecation warnings:
Occurrences: 3
DEPRECATION WARNING: dep2
----------
Occurrences: 2
DEPRECATION WARNING: dep1
----------
You can see it ignored the deprecation warning dep4 and dep3 completely.
I tried to debug this and figured out that the inject is not working as expected.
I did something like this (it could be made better for sure):
extracted_deprecation_warnings = file_paths.map { |path| extract_deprecations(path, pattern) }
deprecation_warnings = {}
extracted_deprecation_warnings.each do |a|
a.keys.each do |h|
if deprecation_warnings[h]
deprecation_warnings[h] << a[h].flatten
deprecation_warnings[h].flatten!
else
deprecation_warnings[h] = a[h].flatten
end
end
end
deprecation_warnings
and this returns
rishi@rishi:~/projects/next_rails$ exe/deprecations info --path "/home/rishi/projects/next_rails/dep_test/*"
Ten most common deprecation warnings:
Occurrences: 5
DEPRECATION WARNING: dep2
----------
Occurrences: 3
DEPRECATION WARNING: dep3
----------
Occurrences: 2
DEPRECATION WARNING: dep1
----------
Occurrences: 1
DEPRECATION WARNING: dep4
----------
There was a problem hiding this comment.
I have not tested my suggested code with all possible scenario. I was just trying to suggest an example for you to play with if it seems correct to you.
There was a problem hiding this comment.
This would be an interesting test case to be added to the test suite.
|
@mateusdeap Let me know if I am understanding this all a bit differently. |
There was a problem hiding this comment.
@mateusdeap Thanks for taking the time to submit this! ❤️
I agree with the comments by @rishijain.
Additionally, I would like to see most of this code moved to a file/s under the lib directory.
Considering you are adding behavior, we need to have a test case to verify that it does what it is supposed to be doing. This will save us time in the long run.
I know that it's a little beyond the scope of this PR, but I think we should not have all that code in the exe/deprecations file as it makes it hard to test with our test suite.
| deprecation_warnings = file_paths.map { |path| extract_deprecations(path, pattern) } | ||
| .inject(:merge) |
There was a problem hiding this comment.
This would be an interesting test case to be added to the test suite.
| opts.on("--path PATH_OR_FILE_GLOB", "Path to deprecation shitlist or glob for multiple files") do |path| | ||
| options[:path] = path | ||
| end |
|
@etagwerker @rishijain Sorry for the late response. I'll later try to address all of these comments |
|
@mateusdeap Sounds good, thank you! |
Description
This is a bit of a 2 for 1:
--path, so that we can provide a custom path to our deprecation files.deprecationscommand a file glob. That way it will print out information on the deprecation warnings present in all the files that match that glob.And that's basically it.
Motivation and Context
The motivation is given on the issue it fixes. Fixes #3.
How Has This Been Tested?
Manually on the command line since the info command is entirely contained in the
exe/deprecationsscript.I will abide by the code of conduct