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

Comic Number and Comic Date #14

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

Comic Number and Comic Date #14

wants to merge 9 commits into from

Conversation

Kixiron
Copy link

@Kixiron Kixiron commented Nov 30, 2018

Added two functions to the Comic class:

Get Date

getDate(dateType="MDY")

Returns the comic's date in one of three formats, defaulting to Month/Day/Year
Possible formats:

  • Month/Year/Day MYD
  • Day/Month/Year DMY
  • Month/Day/Year MDY

More formats are easily added, I just chose the most used ones globally.

Edit: Now the date is returned as datetime

Get Comic Number

getNumber()

Pretty simple, returns the comic's number. Just a nice quality of life thing for people who use the API.

Further Changes:

Moved function definitions before class definitions, mostly to make my editor stop screaming at me.

Formatting

My formatting might be off, as I usually use spaces, but if there are any problems that I missed, they'll be easy enough to fix

Moved functions to before class definitions, allowing using the API to be nicer (now allows the IDE to recognize functions) and for my editor to stop screaming at me
Options for YMD, DMY, and MDY
@TC01
Copy link
Owner

TC01 commented Dec 1, 2018

Thanks for the interest!

For getDate(), I think it would probably be better to return a datetime object (and then have the user decide how to format it)-- does that make sense to you? I think this module probably does not need its own list of date formats.

@Kixiron
Copy link
Author

Kixiron commented Dec 1, 2018

Thanks for the interest!

For getDate(), I think it would probably be better to return a datetime object (and then have the user decide how to format it)-- does that make sense to you? I think this module probably does not need its own list of date formats.

I converted it to datetime now, the only thing is that I had to import the datetime module

@TC01
Copy link
Owner

TC01 commented Dec 3, 2018

Hi @Kixiron,

I believe these changes look okay. However, a few general stylistic comments.

  • It's usually better to break a pull request up instead of adding new, unrelated changes to an existing one (I prefer to work in branches for this reason, rather than working on the master branch of a fork). This is fine for now, but I'd ask you to avoid adding on more new changes for now.

  • It's unfortunate that you moved things around in the source file too. It makes it much harder to review what's actually been changed. Again, though, this is okay for now since the changes are pretty simple anyway.

  • It would be nice if you could squash commits together to make a cleaner git history; for example, things like this commit probably should be squashed into a neighboring commit. I can squash the entire branch manually when I merge ("squash and merge"), but then your master branch and my master branch won't agree. It it isn't too much trouble, it would be really nice if you could go through your commits with git rebase -i, combine some of them (especially the ones with very barebones commit messages), and then force push.

@Kixiron
Copy link
Author

Kixiron commented Dec 4, 2018

I'm really sorry, but I have no idea what to do with git bash, as I usually use the desktop app.

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.

2 participants