Skip to content
This repository has been archived by the owner on Nov 1, 2022. It is now read-only.

Tests #32

Open
wants to merge 43 commits into
base: master
Choose a base branch
from
Open

Tests #32

wants to merge 43 commits into from

Conversation

BlitzKraft
Copy link
Contributor

@BlitzKraft BlitzKraft commented Jul 7, 2016

Still a work in progress. Wrote two tests and they check out.

These are the TODOs I can think of. Feels like there should be more tests.

TODO:

  • Remove downloaded files after testing (clean up)
  • Test for comic out of range (or does not exist)

BlitzKraft and others added 30 commits April 6, 2016 11:21
Download continues, if the comic is listed in the exclude list.
* Description contains alt text too.

* index on master: 7b31df1 Description contains alt text too.

* Now download-all works, with the alt-text.

* Fails if it does not find an image. Examples: 1350, 1663

* Updated readme.

* Updated readme.

* Added exclude list to not stumble on comics without images.

* my bad.

* Updated readme.

* Exclude list for image downloads.

* Added 1350 to exclude.

* Fixed conflicts from exclude list.

* Fixed conflicts.

* Hard coded description for 472. For the sake of completion.

* Removed all comments. Except where non-trivial info is needed.

* Used to cli.py with all the comments. Contains all the comments. Functionally identical.
Changelog
========

- fixed bug which stopped download of comic when there was no image.
- fixed bug which did not download meta data for comic.
- Added logo for the package
- major refactoring
- several bug fixes
Updating my fork of the repo.
* Description contains alt text too.

* index on master: 7b31df1 Description contains alt text too.

* Now download-all works, with the alt-text.

* Fails if it does not find an image. Examples: 1350, 1663

* Updated readme.

* Updated readme.

* Added exclude list to not stumble on comics without images.

* my bad.

* Updated readme.

* Exclude list for image downloads.

* Added 1350 to exclude.

* Fixed conflicts from exclude list.

* Fixed conflicts.

* Merged cleanUp.

* Merged trimFat

* Argparse. And short hand for options.

* Check on refactoring.

* No more docopts.

closes tasdikrahman#10
BlitzKraft and others added 13 commits April 11, 2016 06:23
Updating my fork of the repo.
* Description contains alt text too.

* index on master: 7b31df1 Description contains alt text too.

* Now download-all works, with the alt-text.

* Fails if it does not find an image. Examples: 1350, 1663

* Updated readme.

* Updated readme.

* Added exclude list to not stumble on comics without images.

* my bad.

* Updated readme.

* Exclude list for image downloads.

* Added 1350 to exclude.

* Fixed conflicts from exclude list.

* Fixed conflicts.

* Merged cleanUp.

* Merged trimFat

* Argparse. And short hand for options.

* Check on refactoring.

* No more docopts.

* Show comic using feh.

* Test files removed.

* Uses xdg-open to show comics.

* Updated README with new show option. Needs gif.

Closes issue tasdikrahman#27
Updating my fork of the repo.
Updating my fork of the repo.
* Description contains alt text too.

* index on master: 7b31df1 Description contains alt text too.

* Now download-all works, with the alt-text.

* Fails if it does not find an image. Examples: 1350, 1663

* Updated readme.

* Updated readme.

* Added exclude list to not stumble on comics without images.

* my bad.

* Updated readme.

* Exclude list for image downloads.

* Added 1350 to exclude.

* Fixed conflicts from exclude list.

* Fixed conflicts.

* Merged cleanUp.

* Merged trimFat

* Argparse. And short hand for options.

* Check on refactoring.

* No more docopts.

* Speeling mistake.

* Choose image viewer by changing IMAGE_HANDLER variable. More consistent double/single qoute usage.
Prior to this, working directory remained, for example,
$WORKING_DIRECTORY/xkcd_archive/1000/, after a comic is downloaded. This
is proving inconsistent during testing.

If there is a download, the above is the CWD and the program exits. But,
if there was no download, the CWD does not change. Remains whereever the
command is executed from.
Tests check if there is a file created in the required path. Checks for
both the description and image. Two comics are chosen, one regular and
one from the exclude list.
xkcd_dl/cli.py Outdated
@@ -216,6 +216,7 @@ def download_one(xkcd_dict, xkcd_num):
os.rename(file_name, "{description}.jpeg".format(
description=new_description)
)
os.chdir(WORKING_DIRECTORY)
Copy link
Owner

Choose a reason for hiding this comment

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

any particular reasons for doing that?

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Yes. During testing, if the download succeeds, it does not return to the WORKING_DIRECTORY. And CWD is WD/xkcd_dl/1000/. However, it if the comic is already downloaded, the CWD does not change.

@tasdikrahman
Copy link
Owner

Hey @BlitzKraft

Will keep tabs on further PRs on this and
And hey, good to have you back 😄

@BlitzKraft
Copy link
Contributor Author

BlitzKraft commented Jul 8, 2016

Hey @prodicus. unittest seems to be a part of python natively (correct me if I'm wrong). However, pytest is an additional install. I am comparing both the libraries now.

But requiring to install another library just for testing seems like overkill to me.

EDIT: I take back what I said. pytest is way easier to use.

@tasdikrahman
Copy link
Owner

Hey @BlitzKraft

Saw it idling since the last 2 weeks. Thought I would ping you on this. Are you still working on this PR?

@BlitzKraft
Copy link
Contributor Author

I haven't worked on it since. Was a bit busy. Moved to a new city just last Monday. I still intend to work on it.

@tasdikrahman
Copy link
Owner

Was just checking on it. Sure 👍

@Kickball
Copy link
Contributor

Kickball commented Sep 4, 2016

@BlitzKraft have you had any chance to work on this?

@BlitzKraft
Copy link
Contributor Author

Guys, My apologies. I got busy and haven't worked on it yet. I am not sure when I might get a chance either.

@Kickball, if you are interested, please join in.

@tasdikrahman
Copy link
Owner

Hey guys

@Kickball @BlitzKraft Just checking if you guys are working on this?

@Kickball
Copy link
Contributor

Hey @prodicus
I've not been working on this and will not have time in the near future.
Best of luck working on this feature though!

@Kickball
Copy link
Contributor

Are we happy to close this; given there has not been any activity on this for 18+ months, this PR has loads of conflicts and its purpose was already achieved in #43?

@BlitzKraft @tasdikrahman

@BlitzKraft
Copy link
Contributor Author

@Kickball Yeah, I second that. Unfortunately, I haven't been able to work on this in a long time.

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.

None yet

4 participants