Page MenuHomeSolus

Inital commit of m64py
ClosedPublic

Authored by Staudey on Aug 29 2018, 9:16 PM.

Details

Summary

Initial commit of m64py

Test Plan

Open m64py and load the test ROM from their repository: https://github.com/mupen64plus/mupen64plus-ui-python/raw/master/test/mupen64plus.v64

Diff Detail

Repository
R4530 m64py
Lint
Automatic diff as part of commit; lint not applicable.
Unit
Automatic diff as part of commit; unit tests not applicable.

Event Timeline

Staudey created this revision.Aug 29 2018, 9:16 PM
Staudey requested review of this revision.Aug 29 2018, 9:16 PM
Staudey updated this revision to Diff 9297.Aug 30 2018, 6:48 PM

Remove unnecessary builddep

JoshStrobl requested changes to this revision.Aug 31 2018, 6:25 PM
JoshStrobl added a subscriber: JoshStrobl.
JoshStrobl added inline comments.
package.yml
7
16–18

Should be alphabetically sorted.

19–20

Should be removed.

This revision now requires changes to proceed.Aug 31 2018, 6:25 PM
Staudey updated this revision to Diff 9336.Sep 1 2018, 5:02 AM

Orderd deps alphabetically, removed unused setup part, added additional licenses

Staudey marked 3 inline comments as done.Sep 1 2018, 5:04 AM

Thank you for the review! I changed everything as requested, though I am still unsure about the licenses part. Is it okay like this now, or if it is not, could you maybe point me to another example of a package with multiple licenses like this?

JoshStrobl requested changes to this revision.Sep 22 2018, 7:43 AM

Sorry for the delay in the review.

package.yml
8–14

All the () should be behind # for commenting, otherwise they'll show up in the spec which is undesired.

This revision now requires changes to proceed.Sep 22 2018, 7:43 AM
Staudey updated this revision to Diff 9629.Sep 22 2018, 8:12 PM

Restructure licenses part again, according to Josh's suggestions

Thank you again for the review. I don't mind the delay, since I know you've been busy (and m64py isn't, and shouldn't, be too high on the Solus priority list ^^).

I've changed the licenses part accordingly. No idea why I didn't even think about commenting out that part myself :)

Staudey marked an inline comment as done.Sep 22 2018, 8:14 PM

I would prefer having this built against python 3, python 2 will be retired in a year so I'd suggest to always prefer python 3 for new inclusion in the repository unless there is a blocker.

package.yml
14

You should not put 3 times the same license. Instead do something like this
- CC-BY-SA-3.0 #Mupen64plus & Python snake logos, Controller image

Also you should alphabetize the list

Staudey updated this revision to Diff 9630.Sep 22 2018, 9:37 PM

Use python3, order licenses alphabetically and only mention licenses once

Staudey marked an inline comment as done.Sep 22 2018, 9:39 PM

Thank you for the input, kyrios123. I've changed everything accordingly; totally forgot about using python3, even though I did so with another package I built for myself.

DataDrake accepted this revision.Oct 18 2018, 12:44 AM
DataDrake added a subscriber: DataDrake.

LGTM. Thanks!

This revision was not accepted when it landed; it landed in state Needs Review.Oct 18 2018, 2:17 AM
Closed by commit R4530:d6809e44d03c: Inital commit of m64py (authored by Staudey, committed by DataDrake). · Explain Why
This revision was automatically updated to reflect the committed changes.