Rework lib/fixup.rb to not use an array for renaming and deleting packages #9784
Conversation
commands/remove.rb
Outdated
| @@ -54,7 +54,7 @@ def self.remove(pkg, verbose) | |||
| device_json['installed_packages'].delete_if { |entry| entry['name'] == pkg.name } | |||
|
|
|||
| # Update device.json with our changes. | |||
| save_json(device_json) | |||
| File.write File.join(CREW_CONFIG_PATH, 'device.json'), JSON.pretty_generate(device_json.to_json) | |||
There was a problem hiding this comment.
The previous code was failing in certain cases-- see attatched issue.
There was a problem hiding this comment.
So the save_json function isn't working and that's the root cause?
There was a problem hiding this comment.
The save_json function isn't working in this specific case-- not sure why it was working when we weren't running things from lib/fixup.rb. If I had to guess, its probably something to do with the fact that save_json isn't actually available in commands/remove.rb and is only there due to the leakage of us calling it from bin/crew, and that doesn't happen when we call it from lib/fixup.rb inside bin/crew despite not require'ing the function.
This is why I'm trying to split everything out into files, and then use files in lib for shared function implementations. Otherwise debugging these sort of issues gets pretty complicated.
There was a problem hiding this comment.
I don't think there's a problem with save_json-- the problem is that we're using save_json at all.
As I said:
If I had to guess, its probably something to do with the fact that save_json isn't actually available in commands/remove.rb and is only there due to the leakage of us calling it from bin/crew, and that doesn't happen when we call it from lib/fixup.rb inside bin/crew despite not require'ing the function.
|
I don't understand why using an array is bad? Isn't that simpler and cleaner? |
|
It means we have to have an unnecessary |
|
Compare: { pkg_name: 'qtwebglplugin', pkg_rename: 'qt5_webglplugin', pkg_deprecated: nil, comments: nil },pkg_rename('qtwebglplugin', 'qt5_webglplugin') |
|
Unit tests are failing. |
|
Fixed. |
|
Honestly I'd be happier with the array being put in another file and just read in, as opposed to putting a bunch of Is it not preferred to split data and code? Also, please discuss refactoring in slack before spending a bunch of time implementing. This is a group project, and you need sign-off on how something is being architected from stakeholders. You can't just assume that because you put in the time, that people are going to be cool with it. |
I don't see how moving the array into a different file changes anything?
My thinking was that this: pkg_rename('qtwebglplugin', 'qt5_webglplugin')was preferable to this: { pkg_name: 'qtwebglplugin', pkg_rename: 'qt5_webglplugin', pkg_deprecated: nil, comments: nil },I just don't think an array is suitable for this sort of thing, given that we have to define every variable. In my view, representing this as a function call is better, because we only need to pass the arguments that we have.
Not really sure what you're getting at-- this is a pull request, of course I am asking for sign-off here. This is me asking for feedback and comments on my changes. |
Fixes #9783.
lib/fixup.rbcould use more work, but this is a start.Also,
js91is deprecated but the package file is still in-tree?Tested and working on
x86_64.Run the following to get this pull request's changes locally for testing.