-
Notifications
You must be signed in to change notification settings - Fork 56
Upgrade rose edit to Python 3 #2808
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
base: master
Are you sure you want to change the base?
Upgrade rose edit to Python 3 #2808
Conversation
This comment was marked as outdated.
This comment was marked as outdated.
oliver-sanders
left a comment
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.
Have made a first pass over the code, looking good.
There's 186 commits here! We could do with squashing these down a bit to make it a bit more manageable.
5684ff9 to
4cf6267
Compare
d7fb376 to
07cd350
Compare
c5a6ee7 to
edd7db0
Compare
8d1aef1 to
f345292
Compare
|
There are some flaky tests lurking in the battery at the moment (nothing to do with this PR), the following Mac OS failures can be ignored:
|
This comment was marked as outdated.
This comment was marked as outdated.
This comment was marked as outdated.
This comment was marked as outdated.
This comment was marked as outdated.
This comment was marked as outdated.
44e54eb to
3704bd5
Compare
| Gtk.ResponseType.ACCEPT, | ||
| ) | ||
| self.window = Gtk.Dialog(buttons=buttons) | ||
| self.set_transient_for(parent_window) |
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.
Relates to bug 15:
| self.set_transient_for(parent_window) | |
| self.window.set_transient_for(parent_window) |
| column.pack_start(cell, True, True, 0) | ||
| else: | ||
| column.pack_start(cell, False, True, 0) |
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 think that pack_start only takes 3 vars (including self.)
These extra variables arent in the old code and don't look like they match the reference material
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.
For long term reference, @astroDimitrios explained to me that pack_start can have 2 or 4 (non self) args depending on which widget the message is attached to. 😢
|
Regular tests now passing 👍 The build tests are failing, they need system-level deps to be installed ( The docs tests are failing, not sure what's going on there, I think an Taking a look... |
|
Issuyes 15 and 21 (which I labelled as "must fix") have been fixed by the latest changes (Thanks @benfitzpatrick). I still need to check through everything else. I'll convert the should fix issues into their own tickets once I approve. |
|
Have pushed a commit (6fee8f0) which should fix the tests. The "Build" tests needed a bit of shimming due to the reliance of pygobject on system dependencies (Can't use Conda here it seems). ... Tests all passing. |
|
My latest coverages are at https://wwwspice/~tim.pillinger/cov/rose-edit-live/ |
|
I have achieved 87% coverage with manual testing. I'm going to suggest that further review/manual testing is probably reaching a point of diminishing returns. |
Fix overly long lines Co-authored-by: Tim Pillinger <26465611+wxtim@users.noreply.github.com>
|
Reviewers, please carefully check the diffs to non-rose-edit files:
|
wxtim
left a comment
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.
Sorry I've just double checked the files Oliver mentioned and I have some questions.
oliver-sanders
left a comment
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.
Have dug through the non-rose-edit files and made a shallow code-skim of the whole PR.
A few small alterations to resolve some minor issues and fix the lint check:
github.com/oliver-sanders/rose/pull/new/feature/rose-config-py3
@wxtim, this will need testing, in particular look for broken icons.
| # self.button.connect( | ||
| # "activate", | ||
| # lambda b: self._popup_option_menu( | ||
| # self.option_ui, | ||
| # self.actions, | ||
| # Gdk.Event(Gdk.KEY_PRESS))) |
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.
TODO: Work out what this did and what we should do about it!
(note the FIXME was pre-existing)
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.
Spun out as #2987
|
I too have a PR to this PR, but it's low importance and can be re-targeted at master astroDimitrios#55 |
|
This PR: astroDimitrios#56 undoes a regression from #2288 |



As discussed this change updates
rose config-edit(or justrose edit) to Python 3 / Gtk3.All seems to function par some cosmetic bugs which are outlined in the Issues on my fork.