-
Notifications
You must be signed in to change notification settings - Fork 369
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
Rework lib/fixup.rb to not use an array for renaming and deleting packages #9784
base: master
Are you sure you want to change the base?
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.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Why change 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.
The previous code was failing in certain cases-- see attatched issue.
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.
So the save_json
function isn't working and that's the root cause?
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 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.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
so why not fix save_json?
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 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.rb
could use more work, but this is a start.Also,
js91
is 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.