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

Add congratulatory mock function #44

Closed
wants to merge 1 commit into from

Conversation

floracharbo
Copy link

At the end of the run() function, print a congratulatory message

@DBCerigo
Copy link
Contributor

DBCerigo commented May 29, 2022

Green tick nice! 🥳

Suggestion for next step if you're wanting to take the practice PR further would be to add a unit test, say to check your congratulatory message gets printed when run is ran. You can base it on the other tests that are in test_run.py.

Soz for mad slow reply! Got a bit busy. Back up to speed on stuff now :)

@floracharbo
Copy link
Author

Thanks Dan!! I've been super busy too and am happy to get back to practising now 😄 Did some reading on mock and pytest, I am happy to learn about that. Though I have to say I don't fully understand where the mocker and tmpdir inputs come from (I just saw you could input these from the others tests).

@DBCerigo
Copy link
Contributor

DBCerigo commented Jun 7, 2022

Nice nice - getting one's head around mocking and stuff is tricky, so big ups for getting there.

Not fully understanding where mocker and tmpdir come from is very understandable - I would put that pytest functionality solidly in the "unhelpfully magic" category. They are fixtures, a concept specific to pytesthttps://docs.pytest.org/en/6.2.x/fixture.html - they are often a massive convenience when doing testing.

A few notes on your latest commit:

@floracharbo
Copy link
Author

floracharbo commented Jun 7, 2022

That makes sense, the fixtures were already defined in pytest, hence why I couldn't see where they were defined.

  • Thank you, Dan!
  • I am now using the pytest capsys fixture to make it neater
  • Damn it, I was pretty convinced I had added the .xml file to .gitignore. I tried amending my commit and force pushing a commit without the xml, but that did not work, it only edited the test_run.py file and the xml file was still here. I ended up reverting the whole commit. Sorry if that is messy, but anyways I think one commit for adding the test is enough, we probably don't need to keep track of my previous commit.

@DBCerigo
Copy link
Contributor

DBCerigo commented Jun 8, 2022

Nice nice nice :) Yea git can be a real pain when trying to remove entire files that were mistaken added to tracking.

You could if you wanted to try doing a git rebase --interactive; you can use this to drop the first two commits you no longer need, and then edit commit message of the final commit so that it says "Add mock function with test" or something of your choosing. rebase is a super useful tool for cleaning up/amending your git history. It will likely take a bit of reading/practice before it becomes clear (definitely hit the ? to make it tell you what each command does once you've started the interactive rebase), or at least it took me a while. I use it like all the time now! :)

At the end of the run() function, print a congratulatory message
@floracharbo
Copy link
Author

Ok yes, sorry for the messy commit history, I should have tidied that up now.

Couple of questions though:

  1. Given I had 4 commits I wanted to simplify: (1) mock function, (2) erroneous commit, (3) revert erroneous commit, (4) test function - did you have a preference for dropping the two commits (2) and (3) and then fixing up (4) to (1) as you seem to have suggested, over just squashing the four (1)-(4) commits together? I am guessing the outcome is exactly the same, right?
  2. Do you have strong views against GitHub Desktop? I would usually use that, just using the command line for practice with you really, but you can do these things there directly.

@DBCerigo
Copy link
Contributor

DBCerigo commented Jun 9, 2022

Couple of questions though:

1. Given I had 4 commits I wanted to simplify: (1) mock function, (2) erroneous commit, (3) revert erroneous commit, (4) test function - did you have a preference for dropping the two commits (2) and (3) and then fixing up (4) to (1) as you seem to have suggested, over just squashing the four (1)-(4) commits together? I am guessing the outcome is exactly the same, right?

🤷 no preference I think, given like you say the outcome is the same! The important thing is that one has a good mastery of editing/altering/rebasing their git history to make it as clean and understandable and clear as possible for the future review of the commits (which from your question it sounds like you're doing great on!). So I would go with you own preference :)

2. Do you have strong views against GitHub Desktop? I would usually use that, just using the command line for practice with you really, but you can do these things there directly.

Hmm, maybe not strong views against it, but I think one might be hindering themselves slightly be being reliant on it. The actual version control system we are using is git, not GitHub, so learning git is more valuable than learning GitHub. I.e. if you start working projects hosted on GitLab (or BitBucket ☠️), then you may feel hindered at that point given their may be GitHub specific stuff that doesn't exist on those other platforms. (I don't think it should ever really be a "command line is the only way" mentality, but personally I super enjoy the power and generality of it.)

@floracharbo
Copy link
Author

I see thanks! If you have other mock practices/actual stuff I can have a go at, let me know 👍

@DBCerigo
Copy link
Contributor

DBCerigo commented Jun 9, 2022

You could have a think/go at this #31 if you want something proper to have a blast on :)

@DBCerigo
Copy link
Contributor

DBCerigo commented Jul 7, 2022

@floracharbo wanna close this as now have #46 we're working from?

@floracharbo floracharbo closed this Jul 7, 2022
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