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.

Sign up for GitHub

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

Jump to bottom

Added python Bogo Sort #142

Merged
merged 3 commits into from
Jun 29, 2018
Merged

Added python Bogo Sort #142

merged 3 commits into from
Jun 29, 2018

Conversation

VikingScientist
Copy link
Contributor

I am curious to hear your thoughts on python 2 vs python 3, as discussed in #141

jiegillet
jiegillet reviewed Jun 28, 2018
View reviewed changes
CONTRIBUTORS.md
@@ -1,3 +1,4 @@
Kjetil Johannessen
Copy link
Member

Choose a reason for hiding this comment

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

Hahaha, you put yourself first :D

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Whoops. Sorry 😊

@jiegillet jiegillet added the Implementation This provides an implementation for an algorithm. (Code and maybe md files are edited.) label Jun 28, 2018
@jiegillet
Copy link
Member

The code is fine, let's wait a bit on the discussion outcome, it will need some adjustments if we go with Python 3.

VikingScientist
VikingScientist commented Jun 28, 2018
View reviewed changes
chapters/sorting_searching/bogo/bogo_sort.md Outdated
@@ -24,6 +24,8 @@ In code, it looks something like this:
{% sample lang="js" %}
[import:1-16, lang:"javascript"](code/js/bogo.js)
{% sample lang="hs" %}
[import:1-3, lang:"python"](code/python/bogo.py)
Copy link
Contributor Author

Choose a reason for hiding this comment

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

Messed up. Thought this was "import"-statements which was supposed to be omitted from the inclusion. Turns out it's the other way around. This is the stuff that is actually going to be included.

Copy link
Contributor

Choose a reason for hiding this comment

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

Yup. You can either specify a range to include or import the entire file.

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Fixed. But for some reason, commit f43acc7 seem to be tagged as changed during my rebase. I did not touch it and am pretty sure that it doesn't really matter that it's here.

Copy link
Contributor

Choose a reason for hiding this comment

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

You should be able to get rid of it by running git rebase -i and changing the commit from pick to drop. I can't explain why it's there in the first place, though.

Copy link
Contributor

@zsparal zsparal Jun 28, 2018 •
edited
Loading

Choose a reason for hiding this comment

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

You should be able to do git fetch && git rebase upstream/master where upstream is:

git remote add upstream https://github.com/algorithm-archivists/algorithm-archive.git

Edit: do the git remote add first

@VikingScientist VikingScientist force-pushed the PythonBogoSort branch 2 times, most recently from cd920b6 to e7a418e Compare June 28, 2018 07:49
Butt4cak3
Butt4cak3 requested changes Jun 28, 2018
View reviewed changes
Copy link
Contributor

@Butt4cak3 Butt4cak3 left a comment

Choose a reason for hiding this comment

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

Alright. First off, welcome to the project and thanks for the contribution!

You made a mistake inside the markdown file. Other than that, the bogo code looks fine.

You fiddled around with in bubblesort.py as well. While you're at it, could just fix the indentation in that file? We're going for 4 spaces in Python and that file was supposedly added way before we started to require uniform style.

chapters/sorting_searching/bogo/bogo_sort.md Outdated
@@ -24,6 +24,8 @@ In code, it looks something like this:
{% sample lang="js" %}
[import:1-16, lang:"javascript"](code/js/bogo.js)
{% sample lang="hs" %}
[import:5-14, lang:"python"](code/python/bogo.py)
{% sample lang="py" %}
Copy link
Contributor

@Butt4cak3 Butt4cak3 Jun 28, 2018 •
edited
Loading

Choose a reason for hiding this comment

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

This is not how this works. It should look like this.

{% sample lang="py" %}
[import:5-14, lang:"py"](code/python/bogo.py)

You have them in the wrong order and you also put them inside the Haskell one instead of beneath it. Just move them both up one line and flip them around.

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Is there any way that I can test this file on my local computer? It's generated html, and I have a hard time verifying if what I'm writing here is correct.

Copy link
Contributor

@Butt4cak3 Butt4cak3 Jun 28, 2018 •
edited
Loading

Choose a reason for hiding this comment

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

Quick rundown of how to test changes locally. If you need any help, feel free to ask here or on Discord (linked in the README) if that's your thing.

  1. Install NodeJS and NPM
  2. Install gitbook with npm install -g gitbook
  3. cd into the project directory
  4. Run gitbook install
  5. Run gitbook serve
  6. Visit localhost:4000 in your browser

chapters/sorting_searching/bubble/code/python/bubblesort.py Outdated
number = [random.randint(0, 1000) for _ in range(10)]
print("Before Sorting {}".format(number))
bubble_sort(number)
print("After Sorting {}".format(number))
Copy link
Contributor

@Butt4cak3 Butt4cak3 Jun 28, 2018 •
edited
Loading

Choose a reason for hiding this comment

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

I don't know why you changed this. Most of the code examples in the AAA currently use a main function and the if __name__ = '__main__' method. It's also okay for these parts to differ from algorithm to algorithm. They're not the focus.

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Blargh! That's where that changed ended up. This was supposed to be on another branch. I'll fix

@Butt4cak3
Copy link
Contributor

Butt4cak3 commented Jun 28, 2018 •
edited
Loading

I just realized that there's not even a code import in bubble_sort.md for Python. The Python implementation never actually showed up in the book. Noone noticed.

@VikingScientist VikingScientist force-pushed the PythonBogoSort branch 2 times, most recently from 354da61 to 4a57626 Compare June 28, 2018 08:40
@VikingScientist
Copy link
Contributor Author

There. Think I've fixed it all properly now. The mess I made with bubble-sort ended up in #145 where I both fix that the python implementation didn't show up as well as the indentation.

@Butt4cak3 Butt4cak3 self-assigned this Jun 28, 2018
@Butt4cak3
Copy link
Contributor

Alright. Everything is looking pretty good now as far as my change requests go.

I'd still say we wait until #141 is resolved until we merge this PR (and all the other Python PRs for that matter). That should happen during the next day or two.

@Butt4cak3 Butt4cak3 merged commit 2d04bd7 into algorithm-archivists:master Jun 29, 2018
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Reviewers

@zsparal zsparal zsparal left review comments

@jiegillet jiegillet jiegillet left review comments

@Butt4cak3 Butt4cak3 Butt4cak3 approved these changes

Assignees

@Butt4cak3 Butt4cak3

Labels
Implementation This provides an implementation for an algorithm. (Code and maybe md files are edited.)
Projects
None yet
Milestone
No milestone
Development

Successfully merging this pull request may close these issues.
4 participants
@VikingScientist @jiegillet @Butt4cak3 @zsparal