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

Materials #21

Merged
merged 9 commits into from
Jan 20, 2015
Merged

Materials #21

merged 9 commits into from
Jan 20, 2015

Conversation

jeremywrnr
Copy link
Contributor

Adds the functionality described in issue 9, namely the ability to download files from blackboard directly from the command line.

Also updates the readme to reflect this added capacity.

@jeremywrnr
Copy link
Contributor Author

These commits could and should probably be condensed/flattened into more meaningful ones, as there are backup or output files were occasionally committed ( 15dbc57 ), which would unnecessarily bloat the master branch's history and size. I don't really have experience w/ condensing or flattening commits though, so any guidance or suggestions will be appreciated.

@clehner
Copy link
Member

clehner commented Jan 19, 2015

If you would like to combine some of the commits, in this branch run git rebase -i master to do interactive rebase of commits from master to your current HEAD. Then you will get a list of commits with a keyword for each one. You can reorder the lines to change the order of the commits. Combine commits by changing the keyword of successive ones to squash (to keep the commit message) or fixup (to ignore the commit message for that commit). That will combine the squashed/fixed-up commit with the commit immediately before it (or series of commits, if you squash multiple successive commits) If you e.g. have a commit that adds a temp file, and you squash it with a commit that removes it, the resulting commit should be free of the temp file. After you finish making the selections you will get prompted to update the commit messages for the new squashed commits.

Alternatively, you can squash all the commits into one, by running (I think) git reset --soft master; git add -u; git commit which is equivalent to doing the interactive rebase as above and using the squash keyword for every commit after the first one in the list.

This can apparently also be done using git merge --squash. (I haven't tried that one.)

If you make a mistake and lose a commit just run git reflog and you can find the commit refs that you were previously working on and can reset to.

@jeremywrnr
Copy link
Contributor Author

Thanks, that worked well. Anything else you think that should be done before merging?

@joeljk13
Copy link
Member

2 minor things I'm seeing now:

For consistency in the code with other commands, it might nice to rename get_materials to bb_materials.

Also, I get a sed error around line 1272 - $temp_materials is empty, and sed doesn't like // at the beginning apparently. (The course doesn't have a Course Materials tab, and the command also says that no materials were found after the error, so it's not that bad.)

Neither of these are critical, so I'd say it's good to merge anytime.

@jeremywrnr
Copy link
Contributor Author

Both valid points, changed to bb_materials, and it should exit if temp_materials is empty now.

Anything else? Otherwise, I'll merge and close tonight.

On not having a Course Materials tab, this would probably be fixed by expanding get_assignments, but that's hard because professors can name the tabs whatever they want (I think). Course Materials is probably the default, as it appears to be the most common.

I've opened an issue that may later explore this. #22

@joeljk13
Copy link
Member

I'd say it's ready to merge.

jeremywrnr added a commit that referenced this pull request Jan 20, 2015
@jeremywrnr jeremywrnr merged commit 531a85e into master Jan 20, 2015
@jeremywrnr jeremywrnr deleted the materials branch January 20, 2015 04:08
@clehner
Copy link
Member

clehner commented Jan 20, 2015

👏

@jeremywrnr
Copy link
Contributor Author

👍 thanks for all the guidance!

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