shen-sat commented 4 years ago

Hi @JacobEvelyn, hope you are having a good Xmas! 🎄

I've made the changes we discussed:

changed trigger for setting a default location from moved to _LOCATION_ to to _LOCATION_
I've added functionality (plus tests) to print Default location set to: _LOCATION_ to stdout when a to _LOCATION_ activity is added by the user
I've updated the readme for setting default locations

While making this PR I came across another instance of us using to _LOCATION_ in a test where we don't want to set a default location (this is expected as to _LOCATION_ wasn't used before as a trigger for anything). I removed/amended it, but it did raise the question again of why we are using to _LOCATION_ as the trigger when its such a commonly used phrase, and why we are not using something more explicit (like moved to or set default location to). I can easily imagine users writing I went to Paris for the day or even I went to Paris without wanting to set the default location to Paris.

Just a minor re-push-back from me :) Let me know what you think, happy to discuss changes further 👍

PS is there a way to run one test a time with minitest? I had a look online, but couldn't find a method that didn't involve installing another gem 🤔

Hi there! Thanks so much for submitting a pull request!

Let's just make sure together that all of these boxes are checked before we merge this change:

[x] The code in these changes works correctly.
[x] Code has tests as appropriate.
[x] Code has been reviewed by @JacobEvelyn.
[x] All tests pass on Travis CI.
[x] Code coverage remains high.
[x] Rubocop reports no issues on Travis CI.
[x] The README.md file is updated as appropriate.

Don't worry—this list will get checked off in no time!

codecov[bot] commented 4 years ago

Codecov Report

Merging #245 into master will not change coverage by %. The diff coverage is n/a.

@@           Coverage Diff           @@
##           master     #245   +/-   ##
=======================================
  Coverage   98.38%   98.38%           
=======================================
  Files          23       23           
  Lines         741      741           
=======================================
  Hits          729      729           
  Misses         12       12

Continue to review full report at Codecov.

Legend - Click here to learn more Δ = absolute <relative> (impact), ø = not affected, ? = missing data Powered by Codecov. Last update 0c0a8cf...0c0a8cf. Read the comment docs.

JacobEvelyn commented 4 years ago

Thanks so much for making this PR, @shen-sat ! I totally hear your concerns about an overly sensitive trigger for this. If you're okay with it, let me think on this a bit more. In the meantime, I'll review your PR as-is and leave a few comments!

shen-sat commented 4 years ago

Thanks so much for making this PR, @shen-sat ! I totally hear your concerns about an overly sensitive trigger for this. If you're okay with it, let me think on this a bit more. In the meantime, I'll review your PR as-is and leave a few comments!

No worries @JacobEvelyn , it's always fun working on friends :)

I'm not too concerned about the trigger situation, I just wanted/thought it best to flag it to you as something that has organically come to my attention via contradictory examples in the codebase itself - totally cool to roll with what you decide on this as we've discussed it quite a bit already 😄

I do need a bit of clarification on why Martha's Vineyard is better than Marie's Diner though (see my comments, above). I understand Martha's Vineyard is also made up of multiple words, but is it also a location someone can move to (and therefore set as a default location)? I've also put in some other questions in my comments - once these are resolved I'll implement the required changes 👍

PS did you see my previous question re: executing specific tests in friends? If you are able to do this, what command do you use?

PPS I just realised I didn't answer your question around my being a contributor to friends - I think this might be to do with my choice to make my email private. Let me try making it public and seeing what happens 🤞

JacobEvelyn commented 4 years ago

Hey @shen-sat ! Thanks as always for the thoughtful discussion and careful coding! Looking forward to merging this soon so you can take a stab at some more interesting features! (Or take a break from me if you need one. 😅)

I'm not too concerned about the trigger situation, I just wanted/thought it best to flag it to you as something that has organically come to my attention via contradictory examples in the codebase itself - totally cool to roll with what you decide on this as we've discussed it quite a bit already 😄

I do hear your concern though! I think once we make this change we can see how it feels to use, and whether we get any feedback from users. As an example of something like this in the past, tagging in friends was originally with the # character as in Twitter. It soon became clear that this was the wrong decision (https://github.com/JacobEvelyn/friends/issues/122#issuecomment-219570641) and we fixed it. I'm more than happy to revise previous decisions when new evidence comes to light! (Plus, friends isn't yet at a 1.0 release, so I feel we have a bit more leeway in this department.)

I do need a bit of clarification on why Martha's Vineyard is better than Marie's Diner though (see my comments, above). I understand Martha's Vineyard is also made up of multiple words, but is it also a location someone can move to (and therefore set as a default location)?

Ah, yes, sorry for the confusion! Martha's Vineyard is an island in the northeastern United States—known there as a summer getaway for the wealthy. I couldn't think of any other examples of places that have both punctuation and spaces, but perhaps it would be best to use multiple locations to illustrate this instead?

I've also put in some other questions in my comments - once these are resolved I'll implement the required changes 👍

Will take a look, thanks!

PS did you see my previous question re: executing specific tests in friends? If you are able to do this, what command do you use?

Whoops, sorry for forgetting to respond to this! This is indeed an annoyance I have (for my day job I use RSpec, which is much more full-featured and makes this sort of thing easier), and I haven't found a good way to do it without adding a gem. (There's supposedly a way to specify the entire test name on the command line but it's a lot of trouble and I'm not sure I've ever gotten it to work correctly.) I added the steps I usually take to step 6 here: https://github.com/JacobEvelyn/friends/blob/master/.github/CONTRIBUTING.md

PPS I just realised I didn't answer your question around my being a contributor to friends - I think this might be to do with my choice to make my email private. Let me try making it public and seeing what happens 🤞

I doubt your email needs to be public on your profile, but I found this help article and it says the email you use in your commit (which I won't type here but you can see with git log) needs to be tied to your account. Could your account be using a different email?

JacobEvelyn commented 4 years ago

Hey @shen-sat! Anything I can do to help get this over the line?

shen-sat commented 4 years ago

Hi @JacobEvelyn soooo sorry for the late reply! I've been busy with other projects, and I haven't been keeping on top of my emails/notifications. Totally my bad :(

I'm heading to bed now, but I'll pick this up and (re)read through your comments tomorrow if that's cool? I'll shout if I have any questions

JacobEvelyn commented 4 years ago

@shen-sat let me know if my answers make sense! Happy to discuss anything, as always. And to your question:

Out of curiosity, do you collect much feedback from users? I've never thought to ask: how many people use friends?

Unfortunately, I don't have much contact with users outside of GitHub issues and PRs. I often ask for feedback in issues I create but have trouble getting any input unfortunately. And I don't have a way to see how many users other than the download stats from RubyGems. If you have any suggestions for how I can improve any of this I'd love to hear them.

shen-sat commented 4 years ago

All makes sense, thanks @JacobEvelyn, I'll begin the work 👌

Whoa it's been downloaded over 39 thousand times, that's awesome! Yeah, it's interesting with gems - there isn't anything to measure/monitor (apart from downloads) unlike other apps like websites or mobile apps. If I have any suggestions for tracking use/behaviour, I'll give you a shout 👍

shen-sat commented 4 years ago

Planning to have something for you to review this weekend 🤞

JacobEvelyn commented 4 years ago

I noticed you pushed a few more commits @shen-sat! Just wanted to say let me know when you'd like me to review, and please let me know if there's anything I can help with!

shen-sat commented 4 years ago

Hi @JacobEvelyn! I've been working on this over the weekend - it's a little more complicated than I thought it was going to be, but your pseudo code has really helped me structure the implementation.

I've got a lot of refactoring still to do: the code needs DRYing, and rubocop is complaining about some lines that are too long. I'd therefore like to clean up the code before you officially review it if that's ok?

However, one thing that would help me is if you reviewed specifically the tests now for two things:

Have I covered all the scenearios we discussed?
I've written private methods for the tests for the first time (because I don't think I can use shared examples with minitest) - do my private methods make sense?

If you can give me input for the above to questions, that would super awesome. I can then incorporate your comments while I refactor the rest of the code and then give you the official thumbs up to review 👍 And thanks for being so patient! :)

shen-sat commented 4 years ago

Actually @JacobEvelyn - scratch that! I've just looked at my code after a night's sleep and had a eureka moment 💡 I've thought of a much much better way to re-arrange my tests so that they are a) comprehensive and b) way easier for you (and future devs) to read.

Would you mind holding off reviewing for the moment (ie ignoring my last comment)? I'm busy for the next couple of days, so I'd like to timebox this refactor until the end of this week. After that point, whether I've finished or not, I will have looked at the code for too long and thus it will be time for your review :)

Let me know if that's cool with you!

JacobEvelyn commented 4 years ago

That works for me! ❤️

shen-sat commented 4 years ago

Hey @JacobEvelyn! I'm finally done with this PR (for now).

I’m not super satisfied with my implementation, but as I mentioned in my last comment I think it’s time for a fresh pair of eyes to look at it. Tests are passing, but it could do with some DRYing and rubocop is complaining about lines that are too long. I’ve also used very long (but descriptive!) names for variables and methods - I’ll need to change these too.

Deciding what scenarios to test was the most challenging part of this PR, but it was also really fun to try and figure out. It was actually quite hard to decide when to stop writing tests, because there are potentially many different scenarios a user can find themselves in, especially when adding a default location in the past. I’ve tried to structure the tests like so:

With the activity added being the latest activity
With the activity added being an activity in the past:
- With no proceeding default locations
  - Different preceding activity scenarios
- With no preceding default locations
  - Different proceeding activity scenarios
- With the preceding default location the same as the added activity
  - With same proceeding default location
  - With different proceding default location
- With the preceding default location different to the added activity
  - With same proceeding default location
  - With different proceding default location

I look forward to your review - as alway, very happy to discuss/explain my implementation :)

JacobEvelyn commented 4 years ago

@shen-sat just left a review! Happy to discuss anything, as always!

shen-sat commented 4 years ago

Apologies @JacobEvelyn, crazy busy week at work! Thanks for reviewing, I'll check out your comments on the weekend 🙂

shen-sat commented 4 years ago

Busy week again this week, apologies @JacobEvelyn. I've addressed some comments, replied to others, and haven't responded/addressed the rest. If you are ok with waiting a few more days I will address/reply to the outstanding comments.

JacobEvelyn commented 4 years ago

No problem, @shen-sat! Just let me know when I should take another look! (As a head's up, I'll be doing some travel over the next few weeks but should be able to check here sporadically.)

I'm really looking forward to getting this over the line!

shen-sat commented 4 years ago

Hey @JacobEvelyn - have addressed your comments and pushed latest changes. Code is now ready for your next review :) I've put a few questions/clarifications as well for you to look at.

Even though I'm going with your implementation and I'm deleting a lot of my code, I want to point out that I really appreciate you pointing out refactoring I can do in the soon-to-be-deleted code - super useful learning opportunties for me so please continue them 👍

The elephants in the room now are my rubocop errors 😅 A lot of my test descriptions are super long, but I'm struggling with making them shorter whilst also making them descriptive for future devs. Do you have any suggestions?

shen-sat commented 4 years ago

Hi @JacobEvelyn I've had a look at your comments and they all make sense 👍 Will implement changes hopefully this weekend.

I'm leaning towrads including stable_sort in one place, but I'll have a proper think about it while I implement the other changes.

Haha, yes we are so close! I can see the light at the end of this long (but interesting!) PR tunnel 😎

shen-sat commented 4 years ago

Hey @JacobEvelyn hope you're well! I'm self isolating from the coronavirus atm - it sucks, but the only good thing about the situation is that I can focus on my code projects distraction free 👨🏾‍💻

I've made the changes we discussed. Of note:

I've put stable_sort into the read_file method in Introvert, so now the @activities are sorted before doing anything
I've put in tests for set_implicit_location! and default_activity_output that test against an unordered ###Activities list
There is one test failing - I think it's a test to essentially make sure we print an unordered ###Activities list as an unordered list, though I'm not sure why we'd want this behaviour now that I've moved stable_sort to make sure everything is sorted correctly. Would it be ok to delete this test?

shen-sat commented 4 years ago

Hey @JacobEvelyn - all changes made!

Provided the build passes on Travis CI and I get the thumbs up from you, I'll squash the commits and push one more time - then I think the most epic PR I have ever been a part of is ready to be merged 😄

Glad you are self isolating where you can 👍 It seems like the way to beat this thing is to stop the spread as soon as possible. It's difficult though, especially when it means not seeing friends and family as frequently. The flip side is that because many people are working from home, we all have more time than ever to contribute to open source #silverlining.

And working on friends has become a staple part of my coding life now, so it's something familiar to cling to in these strange times. After a small break I might pickup a new new issue or create a new one - but I'll message you about that separately 👍

JacobEvelyn commented 4 years ago

This looks good to me, @shen-sat! 🎉

Thanks for all the hard work here! Once you squash (or I can squash on my end if you prefer) I'll merge and release a new version!

This ticket ended up being a lot bigger than I ever expected, so a huge huge thanks 🙏 for all your patience and work throughout this process. I'm also really glad you're doing everything you can to fight the pandemic. I am too (didn't mean to imply earlier that I wasn't fully isolating!) and I agree it's just a hard thing that requires a lot of effort from all of us. I'm happy to have you spending your extra time on open source though and friends in particular, and I'm really eager to see what we can get done together!

You've been awesome. Keep it up!

JacobEvelyn commented 4 years ago

Actually, I’ve never used the “squash and merge” button on my end! Do you mind if I try it?

shen-sat commented 4 years ago

Thanks @JacobEvelyn! And as usual, you've been too awesome to work with - I feel like we are developing a good working 'flow' 😊 And please feel free to git squash and merge 👍

Just a few of things I'd like to mention as part of some post-PR feedback:

Please keep up your open, friendly communication style. I mentioned it before, and I'll mention it again: having someone open to discussion around things makes me feel so much more invested in the project
the refactorings you suggest as part of your PR reviews are always super easy to follow. I especially apreciated the pseudo-code you wrote for this PR, it really helped me understand what you were looking to implement
I get carried away sometimes because during our chats we come up with lots of interesting ideas. And if I've had too many coffees I get all excited and try to implement them all in one go 🙃 But I'm realising this isn't the most efficient way to work, so on future PRs I want to continue our super communicative style where we come up with lots of improvements, but I'd like to capture those improvements in separate PRs. Does that sound ok?

And if you have any feedback for me, please let me know!

I'm taking a break from friends for a bit while I try and help out with the coronavirus situation. I actually found a decent site where devs can help out: https://helpwithcovid.com/ so I'm going to sign up to some projects listed there. But I will be back for sure 🙌

JacobEvelyn commented 4 years ago

Hey @shen-sat! Thanks so much for the feedback! I really appreciate your openness with me.

But I'm realising this isn't the most efficient way to work, so on future PRs I want to continue our super communicative style where we come up with lots of improvements, but I'd like to capture those improvements in separate PRs. Does that sound ok?

Yep, that makes sense to me! I think I definitely could have done a better job constraining ideas to keep the scope small. I'll try to be mindful of that in the future.

And if you have any feedback for me, please let me know!

Honestly, you've been great to work with. I really have enjoyed bouncing ideas off of you and seeing the way you approach code. I worry that I'm sometimes too heavy-handed with how I want things done and really appreciate your flexibility in that regard!

I'm taking a break from friends for a bit while I try and help out with the coronavirus situation. I actually found a decent site where devs can help out: https://helpwithcovid.com/ so I'm going to sign up to some projects listed there. But I will be back for sure 🙌

Awesome. I'm so glad you're safe and doing what you can to help others. I'm also doing what I can, and look forward to hearing from you on the other side of all this. Stay healthy! ❤️

Also, I just released your change in version 0.48, which you can get with friends update. I'm personally really looking forward to this feature as it will let me clean up a lot of cruft in my friends.md file. Let me know how you find it, and hopefully we'll get some feedback from others as well!

JacobEvelyn / friends

Change trigger for implicit location from `moved to _LOCATION_` to `to _LOCATION_` #245

Codecov Report