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

Windows compatibility changes + Instructions #21

Open
wants to merge 22 commits into
base: master
Choose a base branch
from

Conversation

seroam
Copy link

@seroam seroam commented Nov 29, 2016

Made some changes in configuration.py to make it work better with Windows. Namely, these are:

  • Added desktop.ini to excludes
  • Added function safe_open_w(file) to replace open(file, 'w') because windows does not automatically create the .config directory and the program crashes once it tries to write your email+name to the config file (which doesn't exist). save_open_w will create the necessary directory - if it isn't present already - and then open the file in write mode.
  • Added a 'How to install' section to README.md to make it easier to people to get started.
  • Added desktop.ini to the example config file (docs/config.example.yaml) and added another source repo to better illustrate how to add more remote directories.

I couldn't figure out how to properly make a numbered list, hopefully you guys can fix that (so it doesn't say 1., 1., 1. for all steps)

@raphiz
Copy link
Contributor

raphiz commented Nov 30, 2016

@seroam Danke für denen Pull-Request! 🎉 Deine Beiträge können wir gut gebrauchen!

Ich werde mir diesen PR noch im Detail anschauen, habe aber diese Woche viel vor, darum werde ich wohl frühstens am Wochenende dazu kommen, sorry.

Copy link
Contributor

@raphiz raphiz left a comment

Choose a reason for hiding this comment

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

Da wir ja Pakete für die Plattformen machen möchten ist diese Art der Installation nicht empfohlen (Keine Updates etc). Darum möchte
ich es auch nicht in der README.

Am besten du Kommst an einem Montag vorbei, dann können wir das Zusammen anschauen.

@@ -117,6 +133,7 @@ def load_config(raise_if_incomplete=False):
if not os.path.exists(config_path):
if raise_if_incomplete:
raise ConfigurationException('Configuration does not yet exist!')

Copy link
Contributor

Choose a reason for hiding this comment

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

Hier bitte den Whitespace löschen.

@@ -4,6 +4,84 @@ WARNUNG: NOCH IST DIESE SOFTWARE IN ENTWICKLUNG - ALSO NICHT FÜR DEN PRODUKTIVE

Besser als der HSR Mapper ;)

##How to install
Copy link
Contributor

Choose a reason for hiding this comment

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

Diesen Teil bitte in /docs/install.md verschieben. Dort gibt es bereits einen Teil für die Installation des Druckers. Ziel ist es, die Installation mittels Pakete oder Installer zu automatisieren.




###Windows
Copy link
Contributor

Choose a reason for hiding this comment

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

Hier bitte nich einen Hinweis anbringen, dass Windows nicht offiziell/vollständig unterstützt ist.

1. Install dependencies
```
$sudo apt-get install git python3-setuptools gcc python3-dev libffi-dev libssl-dev python3-pip -y
```
Copy link
Contributor

Choose a reason for hiding this comment

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

Hier musst du die Code-Blocks einrücken, da sonst Markdown die Nummerierung nicht Checkout (Siehe https://github.com/seroam/connect/blob/master/README.md)

Copy link
Contributor

Choose a reason for hiding this comment

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

Und ein Space zwischen $ und dem Kommand 😉

```
$openhsr-connect edit
```
Enter HSR information, modify config file for your classes (See example [here](https://github.com/openhsr/connect/blob/master/docs/config.example.yaml))
Copy link
Contributor

Choose a reason for hiding this comment

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

Generell würde ich Links nicht mit here oder so benennen, sondern bsp. See example configuration.

```
Sync the specified directories with the script server.

###Bash on Ubuntu on Windows
Copy link
Contributor

Choose a reason for hiding this comment

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

Wann und was ist hier der Unterschied zur "normalen" Windows?!

f.write(config)


#Open file path and create parent directories if necessary as open() doesn't create them on Windows
def safe_open_w(path):
Copy link
Contributor

Choose a reason for hiding this comment

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

Python Methodenkommentare sind unterhalb mit drei "
Also

def safe_open_w(path):
"""
Open file path and create parent directories if necessary as open() doesn't create them on Windows
"""

else: raise

return open(path, 'w')

Copy link
Contributor

Choose a reason for hiding this comment

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

Whitespace bitte löschen

except OSError as e:
if e.errno == errno.EEXIST and os.path.isdir(os.path.dirname(path)):
pass
else: raise
Copy link
Contributor

Choose a reason for hiding this comment

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

raise auf neue Zeile

@@ -103,10 +105,24 @@ def create_default_config(config_path):
username = input('Dein HSR-Benutzername: ')
mail = input('Deine HSR-Email ([email protected]): ')
config = DEFAULT_CONFIG.format(username=username, mail=mail)
with open(config_path, 'w') as f:

Copy link
Contributor

Choose a reason for hiding this comment

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

Gemäss PEP-8 nur eine leere Zeile

@fabianhauser fabianhauser added this to the future milestone Feb 15, 2017
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

3 participants