Skip to content

[ENH] Attempt to fix issues 737, 198, 752, 209, 366, 382, 695 #832

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

Merged
merged 131 commits into from
Jul 1, 2021
Merged

[ENH] Attempt to fix issues 737, 198, 752, 209, 366, 382, 695 #832

merged 131 commits into from
Jul 1, 2021

Conversation

Nalmeder
Copy link
Contributor

PR Description

Please describe the changes proposed in the pull request:

  • We have added the enhancements listed in the above issues, as well as written tests and made sure the continuous integration passed and complied with all linters. Do note: there may be better solutions.

**This PR resolves #737 **
**This PR resolves #198 **
**This PR resolves #752 **
**This PR resolves #209 **
**This PR resolves #366 **
**This PR resolves #382 **
**This PR resolves #695 **

PR Checklist

Please ensure that you have done the following:

Automatic checks

There will be automatic checks run on the PR. These include:

  • Building a preview of the docs on Netlify
  • Automatically linting the code
  • Making sure the code is documented
  • Making sure that all tests are passed
  • Making sure that code coverage doesn't go down.

Relevant Reviewers

Please tag maintainers to review.

Nalmeder and others added 30 commits April 1, 2021 18:43
# Conflicts:
#	janitor/functions.py
@ericmjl
Copy link
Member

ericmjl commented May 26, 2021

@BaritoneBeard it appears there's a conflict. I'm going to resolve it, but you might need to pull down changes before pushing any more changes that you make.

@ericmjl
Copy link
Member

ericmjl commented Jun 1, 2021

@BaritoneBeard how are we doing here? Is there anything we could do to push this PR along? Most of what's failing is just the code style checks; everything else appears to be working fine! 😄

@Nalmeder
Copy link
Contributor Author

Nalmeder commented Jun 1, 2021

@ericmjl
Yeah, sorry about that I planned to work on it but my job has been demanding my free time lately. I plan to work on it on my lunch breaks this week so hopefully everything should be good.

I just need to alter truncate_datetime and then I should be done on the code side.

Also, for code style: darglint in particular was giving me trouble last I worked on this and I couldn't figure out what I was doing differently so I may be pinging you in the future for some assitance. First I'll work on the code, then fix the style when thats done.

@ericmjl
Copy link
Member

ericmjl commented Jun 1, 2021

Awesomeness, @BaritoneBeard! Thanks for letting us know. No pressure, btw, just wanted to make sure the PR didn't get dropped, that's all, especially given the good stuff that's in there. Do make sure you enjoy your lunch breaks too! It's always important to get some rest in between.

@ericmjl
Copy link
Member

ericmjl commented Jun 7, 2021

@BaritoneBeard are you encountering issues with black? If so, let me know if you'd like me to jump on a call with you. I recognize that satisfying a linter is one of the most annoying parts of software development and that working with someone through it is the best way to solve it.

@Nalmeder
Copy link
Contributor Author

Nalmeder commented Jun 7, 2021

Alrighty @ericmjl It seems my code stuff is finished, all thats left are some Linter compliance problems that I can't quite figure out, hoping you or another mod may be able to help out.

  1. After running black, my terminal tells me utils.py has been reformatted, upon committing changes, it states everything is up to date. On push, black fails. Thus it seems I'm in a strange loop where I can't format utils.py and thus the black compliance check will always fail.

  2. I have no idea what darglint wants, I can't quite see what I'm doing differently from other functions, and my initial PR had no darglint errors, so it's not like my group and I did something particularly wrong the first time. This is probably just due to my lack of familiarity with it.

I see you've asked a question before I was able to comment. If a call is best for you that works for me, we can decide a date and time. If you've seen these particular errors before and know a quick solution you can just let me know and I can work with it some more. Whatever's easiest.

@ericmjl
Copy link
Member

ericmjl commented Jun 7, 2021

@BaritoneBeard definitely, let's schedule a call. Can you send me an email via http://shortwhale.com/ericmjl? (I use Shortwhale to protect my email address from the public.)

@Nalmeder
Copy link
Contributor Author

Hey @ericmjl I sent you an email a few weeks ago and haven't heard from you, no rush of course I just wanted to make sure you received it because if not I can simply send another. If you have received it an you're just busy, no worries I will continue to chill.

@ericmjl
Copy link
Member

ericmjl commented Jun 23, 2021

Hey @BaritoneBeard! I searched in my Inbox and couldn't find any email from you. No worries, these things happen. Could you send it again?

@Nalmeder
Copy link
Contributor Author

@ericmjl Sure thing, it's been sent just now so you should have it at this time

@ericmjl
Copy link
Member

ericmjl commented Jun 26, 2021

@BaritoneBeard this is weird, I still haven't seen anything in my inbox sent from you via Shortwhale.

No worries, can you send an email to me directly?

Really sorry we ended up going back and forth on such a trivial matter. I'll want to see whether Shortwhale is working for you when we call too, so I can make a choice on whether I want to keep using it or not.

@ericmjl
Copy link
Member

ericmjl commented Jul 1, 2021

Woohoo, we're all good! 😄

@ericmjl ericmjl merged commit dab4de4 into pyjanitor-devs:dev Jul 1, 2021
@ericmjl
Copy link
Member

ericmjl commented Jul 1, 2021

Writing down the context for future reference: @BaritoneBeard and I did a video call this evening to resolve issues that were preventing the PR checks from passing.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
9 participants