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

Create contributors.json #57

Merged
merged 27 commits into from
Jun 29, 2019
Merged

Conversation

KJLarson
Copy link
Contributor

@KJLarson KJLarson commented Jun 18, 2019

Making sure I understand how to use Git/GitHub in a collaborative environment by submitting a pull request that adds an empty file (I've mostly just used GitHub for my own projects and practice repositories). That seems pretty safe. Am I doing this right?

Progress on #51

@logicalphase
Copy link
Contributor

Looks good, Kari.

@KJLarson
Copy link
Contributor Author

Thanks!

So, now do I keep doing it like this? Working on the add-contributors branch on my forked version of the repository and creating pull requests?

@rviscomi rviscomi added the development Building the Almanac tech stack label Jun 19, 2019
@rviscomi rviscomi added this to the Infrastructure prepared milestone Jun 19, 2019
@rviscomi
Copy link
Member

rviscomi commented Jun 19, 2019

Yep, your workflow is perfect: fork this repository, work on individual features in separate branches, file a PR to merge each branch with the upstream master. 🙌

Only suggestion I can think of is to reference the issue you're addressing in the PR description. GitHub will add annotations linking them together to make it easier to find. For now just add Progress on #51. In the future if the issue should be closed after the PR is merged you can add Fixes #51 and GitHub will do it automatically.

Let's leave this PR open and have you push a couple more commits to your branch to build out the feature. We'll coordinate in #51.


One general tip since you're not used to working collaboratively. Remember to pull from the upstream master before you file the PR, to merge in any changes others may have added. In this case everything looks great.

Check your remotes by entering git remote -v and you'll probably only see your fork labelled "origin". You can add the upstream repository as another remote by entering git remote add upstream [email protected]:HTTPArchive/almanac.httparchive.org.git. Now, whenever you want to merge changes from this repo with your local fork, you can just enter git pull upstream master. And you can keep your origin up to date with git push.

@rviscomi rviscomi self-requested a review June 19, 2019 11:53
Copy link
Member

@rviscomi rviscomi left a comment

Choose a reason for hiding this comment

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

See #51 for next steps

@KJLarson
Copy link
Contributor Author

I think I understand how to do the contributors.html file, but the python files are tripping me up! I'm probably totally off with that stuff, but I tried to work it out. I would love feedback and/or some hints (maybe something for me to research that would help me figure it out?). Thanks!

@rviscomi
Copy link
Member

Thanks @KJLarson this is looking great! Very close to the finish line, just left a few comments and then it should be ready to go.

@rviscomi
Copy link
Member

@KJLarson let me know if you get stuck with any of this. I'm available to walk you through it over chat.

@KJLarson
Copy link
Contributor Author

Is there a way to view all the suggested changes for a file together rather than broken up into all these separate bits? I'm having a hard time making sense of all this and figuring out how it goes together. I'm sure several nights of poor quality sleep doesn't help (:-/)
Thanks!

@rviscomi
Copy link
Member

Try this view: https://github.com/HTTPArchive/almanac.httparchive.org/pull/57/files?utf8=%E2%9C%93&diff=split

@KJLarson
Copy link
Contributor Author

Another question...do you have any suggestions on how specific to be with commit messages? It seems it wouldn't be too helpful (in most cases) to just say "update filename."

@rviscomi - I really appreciate your feedback, patience, and question answering!!! Thanks!

@rviscomi
Copy link
Member

do you have any suggestions on how specific to be with commit messages? It seems it wouldn't be too helpful (in most cases) to just say "update filename."

Naming things is never easy 😁. For commits I like to have a short 1-liner describing what it is that I did. For more complex commits with many changes, I also include a list of each major change within the commit. For example:

add support for new widget

- optimizes widget performance on mobile
- removes redundant call to drawWidget in widget.js
- adds authentication to SuperWidget users

GitHub will name the commit based on the first line and add the rest of the commit message to the description. For small commits don't be afraid to be brief.

@KJLarson
Copy link
Contributor Author

I think I'm stuck on main.py and contributors.py.

For contirbutors.py, I can't tell if it's done or if there are more changes needed.

For main.py, I can't tell if:

  • the @app.route for contributors on line 28 is complete and I can delete the @app.route I added on line 53, OR
  • if they need to be combined together, OR
  • if the @app.route on line 31 isn't done, but the stuff I added isn't what it needs.

Copy link
Member

@rviscomi rviscomi left a comment

Choose a reason for hiding this comment

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

Just a few small changes left!

@rviscomi
Copy link
Member

Once these changes are finished, the only thing left to do is to test out the page by visiting http://127.0.0.1:8080/2019/contributors with the development server running. You should see the info for the 3 contributors in the config file.

@KJLarson
Copy link
Contributor Author

KJLarson commented Jun 27, 2019

Well, shoot. I tested it and got this:
NameError: name 'contributor_util' is not defined

It looks like it's referring to line 33 in main.py.

Edit: I think I see the issue...when we imported it, we used "contributors_util", but when we returned it, we used "contributor_util". I'll change it!

@rviscomi
Copy link
Member

image

Ok @KJLarson there comes a time in every open source contributor's life when they have to face the dreaded merge conflict. For you, today is that day! 😄

#60 was just merged, which affects main.py. You've also made changes to that file, the same lines in fact, so now git doesn't know how to properly merge them after doing a git pull git://github.com/HTTPArchive/almanac.httparchive.org.git master (or similar, depending on how you set up your remote repos locally).

After syncing to master you'll see this error message in the terminal:

Auto-merging src/main.py
CONFLICT (content): Merge conflict in src/main.py
Automatic merge failed; fix conflicts and then commit the result.

If you open up main.py you'll see this:

def contributors(year, lang):
<<<<<<< HEAD
    # TODO: Get contributor data and pass into the template.
    return render_template('%s/%s/contributors.html' % (lang, year), contributors={}, supported_languages=supported_languages)
=======
    contributors=contributors_util.get_contributors()
    return render_template('%s/%s/contributors.html' % (lang, year), contributors=contributors)
>>>>>>> 4ec6c11f9ef25c73c71cff37f1d5e80a663cecd4

There are two sections to this, the part between HEAD and === (the latest contents of the file) and the part between === and >>> (your changes to the old file). To resolve the conflict, we need to merge the two sections while preserving old and new changes. Here's how I would merge them:

def contributors(year, lang):
    contributors=contributors_util.get_contributors()
    return render_template('%s/%s/contributors.html' % (lang, year), contributors=contributors, supported_languages=supported_languages)

(Note that none of the <<< === >>> scaffolding should exist in the code after merging)

After saving that change, if you run git status you'll see this:

Unmerged paths:
  (use "git add <file>..." to mark resolution)

	both modified:   src/main.py

So run git add src/main.py and git commit which will generate a merge commit. You don't need to modify the commit message, it already contains info about what you're merging. After pushing, the conflict should be resolved and the PR will be ready to merge with the master branch.

rviscomi pushed a commit that referenced this pull request Jun 29, 2019
* Create contributors.json

* Add data structure and samples. Progress on #51

* Add code to get and display contributors data. Progress on #51

* Remove import jsonify

Co-Authored-By: Rick Viscomi <[email protected]>

* Update render_template in src/main.py. Progress on #57.

Co-Authored-By: Rick Viscomi <[email protected]>

* Restructure team data. Progress on #57

* Add method to update contributors. Progress on #57

Co-Authored-By: Rick Viscomi <[email protected]>

* Declare global variable first

Co-Authored-By: Rick Viscomi <[email protected]>

* Remove comments

* Add loop to render contributor's team. Progress on #57

Co-Authored-By: Rick Viscomi <[email protected]>

* make user IDs lowercase

* remove contributors GitHub and Twitter

* Add directories for sql queries and sql queries for first metric in each chapter. Progress on #62

* Delete contributor changes. Progress on #65

* Remove contributors.json. Progress on #65
src/main.py Outdated
@@ -31,9 +30,7 @@ def outline(year, lang):
@app.route('/<lang>/<year>/contributors')
@validate
def contributors(year, lang):
def contributors(year, lang):
contributors=contributors_util.get_contributors()
Copy link
Member

Choose a reason for hiding this comment

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

Whoops. This commit totally rolls back the progress on this issue. Can you add these changes back in and only delete the duplicate contributors function definition above?

Copy link
Contributor Author

Choose a reason for hiding this comment

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

🤦‍♀️ Oh my, I think I'm getting confused. It's tricky looking at the changes and suggestions with the green and the red!
Is this what we want?:

def contributors(year, lang):
    contributors=contributors_util.get_contributors()
    return render_template('%s/%s/contributors.html' % (lang, year), contributors=contributors, supported_languages=supported_languages)

Copy link
Member

Choose a reason for hiding this comment

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

Yes, that's it! Also make sure to keep the "import contributors" bit above.

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Where is the "import contributors" bit?

Thanks for your patience!

Copy link
Member

Choose a reason for hiding this comment

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

image

This part is required because the contributors method calls contributors_util.get_contributors().

Copy link
Member

@rviscomi rviscomi left a comment

Choose a reason for hiding this comment

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

LGTM!

@rviscomi rviscomi merged commit 49fdcf8 into HTTPArchive:master Jun 29, 2019
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
development Building the Almanac tech stack
Projects
None yet
Development

Successfully merging this pull request may close these issues.

3 participants