Skip to content
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

chore/documentation code & readme #3

Open
wants to merge 1 commit into
base: master
Choose a base branch
from

Conversation

Teufelchen1
Copy link

Hi,

I just wanted to test out my dongle, accidentally made this PR as a side effect.

In summary:

  • pipfile/pipenv to ease future development
  • enhanced the README accordingly
  • minor cleanup in the code

REGION_LOOKUP = ["region-free", "North-America",
"Europe", "Southeast Asia",
"Latin America", "Africa",
"China", "MPAA", "International"]
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Are these confirmed to match the region in the dongles?
Why is one of them in all lowercase, but then regions use proper uppercase?

The full list also implies that we have encountered all of those in the real world (which likely isn't the case).
I think it's better to add a dictionary with information, or to just not print this information at all (instead, documenting it on the wiki where we can eventually make sense of it, once confirmed).

@@ -44,6 +49,7 @@
f.write(data)
remaining -= chunkSize
cursor += chunkSize
print("Size: "+ str(code_length) + " bytes(written to dvd-dongle-rom.bin)")
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Please add space between "bytes" and "(written".

Maybe just " bytes (dvd-dongle-rom.bin)".

To avoid issues in the future, we might also want to have a temporary variable for the filename (as it's used more than once, and might be changed eventually).


Make sure you satisfy the following requirements:

* basic python know-how
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

This line should be removed - it's expected when people look at github.

(On a somewhat related note: We might eventually have a CI version which is bundled into a Windows executable, but that's not relevant for now)

* pipenv
* libusb1
* a way to attach the DVD Kit dongle to your computer i.e. this [[adapter]](https://www.amazon.ca/Mcbazel-Female-Controller-Adapter-Cable/dp/B000RT2868).
* the dongle itself :)
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

  • No smileys in README please.
  • Capitalize beginnings of sentences

* basic python know-how
* pipenv
* libusb1
* a way to attach the DVD Kit dongle to your computer i.e. this [[adapter]](https://www.amazon.ca/Mcbazel-Female-Controller-Adapter-Cable/dp/B000RT2868).
Copy link
Member

@JayFoxRox JayFoxRox Nov 14, 2019

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

  • GitHub markdown should be using [link description] and not [[link description]]?
  • Use amazon.com (instead .ca) if you must

@@ -1,5 +1,34 @@
A tool to dump the ROM of the Xbox DVD Movie Playback Kit dongle.
## A tool to dump the ROM of the Xbox DVD Movie Playback Kit dongle.
Copy link
Member

@JayFoxRox JayFoxRox Nov 14, 2019

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

This is the first headline, so it should be #, not ##.
Also remove trailing dot/period if it's no longer a real sentence.

I'd suggest to keep this as a sentence instead.

If we really want to have a headline, we should just use # dump-dvd-kit

Make sure you satisfy the following requirements:

* basic python know-how
* pipenv
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I have never used it.

https://github.com/pypa/pipfile implies that it isn't finalized yet?

```

```sh
pipenv install
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I assume this would have to be run in the cloned git repo folder?


### Setup

macOS only:
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Should be more explanatory to avoid giving the impression that this only works on macOS.
Also Windows and Linux also need specific instructions. In fact, I believe @kl0wn struggled to find a working libusb for Windows in recent past.

@JayFoxRox
Copy link
Member

Please adopt the repos standard convention for commit titles (sentence starting with uppercase letter, that describes the change with a verb).
Ideally split independent changes (pipfile / code improvements / readme changes) into separate commits.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Development

Successfully merging this pull request may close these issues.

2 participants