Git - Githubissues

mblayman / markwiki

:warning: UNMAINTAINED: A simple wiki using Markdown

BSD 2-Clause "Simplified" License

29 stars 14 forks source link

Git #27

Closed deadc0de6 closed 8 years ago

deadc0de6 commented 8 years ago

Your project is great ! This branch integrates versioning of the wiki pages using a local git repository (only for the sub-directory wiki). I have also added some pages (history.html and view_history.html) that allow to review old versions of each page and to revert them if needed.

mblayman commented 8 years ago

Hi, @deadc0de6. Thanks for another contribution! I don't have much time for a full blown review at the moment, but here are some general thoughts that I can give you.

sh looks like a proxy for whatever a user has installed. Therefore, the Git integration should be off by default because we can't assume that git is installed and available on the PATH.
View testing for Flask is kind of a pain (I know because I looked), but could you consider adding some unit tests for some of these other core classes (e.g., GitIntegration)? It would be beneficial for catching regression failures in the future.
Thank you for documenting the options in the appropriate config files. Since integrating with git is a pretty awesome feature, could you document the feature in the project features page? https://github.com/mblayman/markwiki/blob/master/markwiki/templates/MarkWiki/Features.md

deadc0de6 commented 8 years ago

Thanks. I have fixed all your points. I tried to add a more or less comprehensive unittest for GitIntegration. If you plan on integrating that to master, I would add it to the project description since this is a nice feature. Feel free to let me know if anything else is missing.

mblayman commented 8 years ago

As I mentioned in the other PR, I think docstrings need English capitalization and punctuation. Rather than call out each docstring, I'll leave this as a general comment that I think all of those docstrings should be touched up.

mblayman commented 8 years ago

Hey @deadc0de6, thanks again for this work! I added some comments. I hope my brief comments are not mistaken as being rude. I find it better to be concise for clarity.

Once those comments are cleaned up, I'll be happy to merge.

deadc0de6 commented 8 years ago

Happy to fix but this sounds a bit pedantic to me.

mblayman commented 8 years ago

Thanks for the feedback. I have high expectations of code quality and style so that future readers have a clear view of the code. I'll update the contributor guidelines so that other future contributors have an idea about my expectations.

mblayman commented 8 years ago

Hey @deadc0de6, I just pushed out version 1.6 with all your contributions. Thanks for your efforts!