Skip to content
This repository was archived by the owner on Dec 1, 2021. It is now read-only.

Conversation

@colbygk
Copy link
Contributor

@colbygk colbygk commented Oct 20, 2017

I was unfamiliar with working against Reddit's APIs, so please feel full freedom to reject or request alterations to this pull request.

What I found was:

  • It appears that the praw api has evolved since post-challenges.py was created and I have updated post-challenges.py according to what I believe is the expected behavior.
  • As a test, I have added in challenge 336, which post-challenges.py pulled from Reddit.
  • In a future pull request, I would like to update the documentation to provide guidance on how to use praw and the setting up of a reddit api access key, as that information was not in a convenient single location.
    • I would also like to update the documentation about the reference to a post_challenges script that seems to no longer exist in the repo and another script referenced inside of post-challeneges.py that is also not there (send-data.sh)
  • After we have discussed these updates, I will submit more pull requests that fit what you would prefer.
  • The naming convention appears to have shifted and challenges appear in the repo that do not follow what is requested in CONTRIBUTING.md, i.e. #336 vs 0336 and so on. Would be happy to rename and resubmit those to follow the naming convention

chals = sub.get_new(limit=num_challenges)
_chals = sub.get_new(limit=num_challenges)
chals = sub.new(limit=num_challenges)
_chals = sub.new(limit=num_challenges)
Copy link
Owner

@freddiev4 freddiev4 Oct 21, 2017

Choose a reason for hiding this comment

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

👍 for update to match the current PRAW API

Copy link
Owner

@freddiev4 freddiev4 left a comment

Choose a reason for hiding this comment

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

Hey @colbygk awesome work. I have some feedback for you below.

In regards to the naming scheme, see if you can run transform.py which should end up renaming directories in the proper format.

@@ -0,0 +1,2 @@
praw>=5.1.0
pprint>=0.1
Copy link
Owner

Choose a reason for hiding this comment

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

👍 for adding a requirements.txt file.

Can we move this to a etc/ folder?


logging.info("Started sending data script")
os.system("./send-data.sh")
# os.system("./send-data.sh")
Copy link
Owner

Choose a reason for hiding this comment

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

I think we can just remove this line along with the log above it

@colbygk
Copy link
Contributor Author

colbygk commented Oct 22, 2017

Hi @freddiev4 I have submitted an update to transform that I think will help it do a better job. I have also backfilled the challenges to fill the gap between 313 and this current week. I just noticed that there appears to be no 334 easy challenge (in the reddit itself)

transform.py Outdated
# Found a likely looking number
# tokens now possess' the remaining title
# a_token now has the original and possible multiple iteration number
# such as 166b
Copy link
Owner

@freddiev4 freddiev4 Oct 22, 2017

Choose a reason for hiding this comment

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

Just as a style thing, we should probably line up the first word of every line to be in the same column

    # Found a likely looking number
    #   tokens now possess' the remaining title
    #   a_token now has the original and possible multiple iteration number
    #     such as 166b

Changed to

    # Found a likely looking number
    # tokens now possess' the remaining title
    # a_token now has the original and possible multiple iteration number
    # such as 166b

For every set of comments

@freddiev4
Copy link
Owner

freddiev4 commented Oct 22, 2017

@colbygk just had one comment in regards to style. If you fix that I think we can go ahead and merge this 🙂 great work on refactoring transform.py!

@colbygk
Copy link
Contributor Author

colbygk commented Oct 22, 2017

Okay, I think that's it. I did update the README.md that you may want to double check.

@colbygk
Copy link
Contributor Author

colbygk commented Oct 22, 2017

Okay, fo'reals, I'm done.

Copy link
Owner

@freddiev4 freddiev4 left a comment

Choose a reason for hiding this comment

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

Added one more comment + you should squash your commits 👍

**post-challenges.py runs on Python 3.x**
```bash
$ cd DailyProgrammerChallenges
$ mkdir tmp
Copy link
Owner

Choose a reason for hiding this comment

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

What are we using tmp/ for here? From what you added above, it looks like we should cd into it.

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Right you are. Also added a bit about using praw.ini

@colbygk
Copy link
Contributor Author

colbygk commented Oct 22, 2017

I am unfamiliar with commit squashing. Recommendation on what to do to squash?

@colbygk
Copy link
Contributor Author

colbygk commented Oct 22, 2017

Hm, I think I figured it out and squashed the last 5 commits into one.

@freddiev4
Copy link
Owner

freddiev4 commented Oct 22, 2017

@colbygk here's an official guide on Rewriting History in Git.

The short of it is that you can do:

git rebase -i @~15 which will allow you to do an interactive rebase (-i), on the head of your branch (@), up until the last 15 commits (~15).

From there you'll see something like:

pick f7f3f6d changed my name a bit
pick 310154e updated README formatting and added blame
pick a5f4a0d added cat-file

And you can change everything below the first commit to:

pick f7f3f6d changed my name a bit
fixup 310154e updated README formatting and added blame
fixup a5f4a0d added cat-file

Which will remove the commit message of every commit but the first (which is 15 commits before the most recent one, the head of your branch), and also squash everything into just one commit

@colbygk
Copy link
Contributor Author

colbygk commented Oct 22, 2017

Looks like since I used squash instead of fixup it melded instead of replacing.

@freddiev4
Copy link
Owner

After you squash, I think you'd need to run git push -f, as it looks like there are still 17 commits here 🙂

image

@colbygk
Copy link
Contributor Author

colbygk commented Oct 22, 2017

Well, now we're down to 11 commits. :) Were you thinking that it should have more squashing?

@freddiev4
Copy link
Owner

freddiev4 commented Oct 22, 2017

Yeah usually we can squash many commits down into 2 or 3 commits.

Here I was thinking something like:

Update most recent challenges
Refactor post_challenges and transform.py
Update config, dev files, and docs

freddiev4 and others added 4 commits October 21, 2017 23:14
The existing transform looked like it could use some improvement. This seems to work on all of the existing challenges, and also handles dealing with challenges that have unexpected numbers embedded, like a date.
@colbygk
Copy link
Contributor Author

colbygk commented Oct 22, 2017

Hmm... Okay... maybe done now? Sorry for the thrashing.

@freddiev4
Copy link
Owner

Looks good to me 👍

@freddiev4 freddiev4 merged commit f231fc4 into freddiev4:master Oct 22, 2017
Sign up for free to subscribe to this conversation on GitHub. Already have an account? Sign in.

Labels

None yet

Projects

None yet

Development

Successfully merging this pull request may close these issues.

2 participants