152/add set home command to set your own location

[shen-sat]

Hey @JacobEvelyn - apologies for the delay in getting this done! Busy with the hustle and grid of life :)

I've added the implementation discussed in: #152

Summary of changes:

• Event now has an implicit_location attribute
• When friends.md is read, if an activity includes the words moved to _LOCATION_, then LOCATION is saved and applied as the implicit_location for any subsequent activities that don't have a location in their description (ie that don't have an explicit location).
• If another moved to _LOCATION_ activity exists further along in the friends.md activity timeline, then this new LOCATION is set as the implicit_location for subsequent activities without an explicit location.
• If the activity moved to _LOCATION_ does not appear in friends.md, then activities that don’t have an explicit location do not have an implicit_location set.

This feature has been tested to work with friends list activities —in and friends list favourite locations commands.

We originally agreed on to _LOCATION_ as the key words to trigger saving and applying implicit_location, but this would mean phrases like went to _Marie's Diner_ would mistakenly get picked up and set as the implicit_location - hence I've changed the key words to moved to _LOCATION_, which is more explicit/specific.

I've redefined CONTENT specific for the tests - I tried to use the existing definition of CONTENT, but it was proving too tricky. I needed CONTENT that would have multiple moved to _LOCATION_ lines to test that implicit_location is set in the correct way.

---

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:

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

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

[codecov]

Codecov Report

Merging #243 into master will increase coverage by 0.03%.
The diff coverage is 100%.

@@            Coverage Diff             @@
##           master     #243      +/-   ##
==========================================
+ Coverage    98.3%   98.34%   +0.03%     
==========================================
  Files          23       23              
  Lines         709      724      +15     
==========================================
+ Hits          697      712      +15     
  Misses         12       12

Impacted Files	Coverage Δ
lib/friends/event.rb	100% <100%> (ø)	⬆️
lib/friends/introvert.rb	98.61% <100%> (+0.02%)	⬆️

---

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 3e63a6c...09b80ff. Read the comment docs.

[JacobEvelyn]

Hey @shen-sat ! Thanks so much for working on this. I'm very impressed with the thought that went into it, the cleanliness of the code, and the very thorough test! Just a few small suggestions, but this is really really nice!

[JacobEvelyn]

Just wanted to apologize for the delay; I'm traveling a bunch right now but will try to review this over the next few days if I can!

[shen-sat]

Hey @JacobEvelyn, hope you are having fun on your travels! I've had lots of fun working on this PR via using pry 🤓

I've addressed most of your comments by making the relevant changes and pushing to this branch. However there are some where I need some more clarification, or that required a little discussion.

Thanks for giving me the heads-up that you're busy - I'll await your comments before proceeding :)

[JacobEvelyn]

Just left some more comments! Thanks for such thoughtful discussion!

[shen-sat]

Hey @JacobEvelyn! Hope you're enjoying your travels :)

I've made all the changes you suggested (see my comments). I don't know why but some of your comments have been stated twice on the PR timeline, but you should see a reply/resolution from me to all of them (I've decided I really don't like Github's UI for PRs 😕. Have you used any other version control platform that has a better way of managing comments? I'm not suggesting it for this project, just curious 🙂).

Importantly, the last commit is breaking the build and I don't know why (see comments). I've looked at the failing test, and it doesn't have anything to do with its associated commit. Would you mind taking a look? I've reverted the commit for now.

[JacobEvelyn]

Hey @shen-sat! Sorry for the delay!

I don't know why but some of your comments have been stated twice on the PR timeline, but you should see a reply/resolution from me to all of them (I've decided I really don't like Github's UI for PRs 😕)

Ick, agreed! 😞

Have you used any other version control platform that has a better way of managing comments?

I used to use Bitbucket a lot for work, and while it certainly has its own quirks (the ecosystem is much worse, and in general it seems to be designed primarily for hosting Java/JVM code), the PR process is much nicer in many small ways. Unfortunately, I don't get to use it anymore 😭

I've also heard good things about GitLab but never tried it.

Importantly, the last commit is breaking the build and I don't know why (see comments). I've looked at the failing test, and it doesn't have anything to do with its associated commit. Would you mind taking a look? I've reverted the commit for now.

Really appreciate you digging in here and proactively reverting the commit as well! You are by far the most thoughtful and professional contributor I've had here! 😄

I don't see why that change would affect that test either, which is making me think it's a nondeterministic test failure. Of course I don't want nondeterministic test failures in general, but since it seems unrelated to your changes and I haven't seen it before, I think it makes sense for me to dig in separately but not block your changes on that test. In the meantime I re-ran the job on Travis on that commit and it passed (note now that that commit in GitHub now has the green check), so if you'd be so kind as to re-revert and get back to the set_implicit_locations! version I think we'll be good to merge this!

[JacobEvelyn]

Oh also, as a heads up—to keep the git history clean I was planning to "squash and merge" or "rebase and merge" your PR and write a cleaner commit message. But if you'd rather do that on your end and squash all of the commits into one so I can just merge that works too! Whichever is easier for you is fine by me.

[shen-sat]

Hey @JacobEvelyn, it's been too awesome working on this PR. I've learnt so much about Ruby and open-source etiquette 😄 And a lot of that has been down to you being a super helpful and considerate maintainer. I've really appreciated the space to have discussions and the chance to push back on things, even though I'm new to your project and new to OS in general.

I will let you figure out the non-deterministic test issue, and I'm happy to squash my commits into one with a useful commit message. And once you merge this will be my first feature contribution to open-source - woop! 🙌

Two more things:

• Do you have any constructive feedback for me? I'd like to continue to contribute to OS, so any feedback to help me improve my code/interaction with maintainers would be greatly appreciated 👍
• I'd quite like to pick up another issue for this project if that's cool with you (maybe after a little break)? I know the code base fairly well-ish now, so it'd be a shame if I didn't put that knowledge to use again. Plus I love the app and all its tests! 🤓

[JacobEvelyn]

Hey @shen-sat! Sorry for the delay on this. I just found one other small thing to change (an unnecessary next), but once that's fixed this looks ready to go (feel free to either make that change in a separate commit or squash it into the existing one, whichever you prefer). I also responded to a comment of yours that I hadn't seen earlier because the conversation was marked as resolved.

I've learnt so much about Ruby and open-source etiquette 😄 And a lot of that has been down to you being a super helpful and considerate maintainer. I've really appreciated the space to have discussions and the chance to push back on things, even though I'm new to your project and new to OS in general.

Hearing this makes me so happy! I'm not sure how apparent it is but one of my goals for this repo is for it to be a place where anyone can get practice contributing to open source. As a result, I try to resist the temptation to work on some tickets myself (like the ones labeled good first issue) so that others can have a go. So I'm really glad to hear that for you at least it seems like this worked! 🎉

Do you have any constructive feedback for me?

This might not be what you mean by "constructive," but here's what I have:

1. Working with you has been nothing but a pleasure for me.
   • Your code is very polished.
   • Your comments are thoughtful.
   • You've been patient with me taking days to respond.
   • You've been patient and interested in the long conversations to pin down exactly how this behavior should work (this is where I imagine many others would get combative or give up).
   • You've been open to my suggestions but also unafraid to push back.
   • In short, working with you has been one of the best open source interactions I've had anywhere, ever. Thank you! 🤗
2. The fact that you're asking for constructive feedback is such a great sign. You've got a bright future ahead of you!

I'd quite like to pick up another issue for this project if that's cool with you (maybe after a little break)?

Yes, absolutely! I'll always be happy to have your help! Depending on what you're looking for, here are some you could consider:

• Easy: Punctuation swallowed after friend name with last initial #235
• Larger but clear: Allow tags to be hierarchical #192
• Larger and need help fleshing out: Add birthdays? #176, Add calendar command to see in which days activities were logged #173
• Very undefined: [PLACEHOLDER] Support more historical relationship tracking #159

Lastly, I'd really appreciate any feedback you have for me! (A note: I am always receptive to blunt/critical feedback—no need to try to spare my feelings!)

[shen-sat]

Thanks so much for the feedback @JacobEvelyn - I felt really happy and super proud after reading it! 😊 Likewise, this PR will the high bar I judge all other PRs against because you've been such a great maintainer to work with.

Positive Feeback (some of this I've said before):

• You've been super welcoming to me as an open-source newbie. I can't pick anything out in particular, it's just your general writing/communication vibe :)
• You always explained your thoughts clearly (no super-technical jargon, no vague statements)
• You invited push back and thoughtful discussion, this is really important for people like me who feel they need 'own' the feature in order to give it 100% care and attention
• You were patient with our back and forth when it came down to defining the feature for this PR
• The checklist you have in the description for PRs is very clear and means less thinking for the contributor
  Constructive feedback:
• this one's really difficult, because I've had such a good experience! The only thing I can think of (and this is scraping the barrel a bit), but this is a really cool repo to work on as an OS first-timer, but I only came upon it by sheer chance. I searched for good first issue and scrolled through a few pages and the word friends stuck out and made me click on the repo to find out more. If there is any way for you to get this repo and/or its issues in front of the community, especially OS first-timers, I think you should go for it.

I'm definitely going to come back to this repo (probably in the new year) and I'll tackle one of the issues you've highlighted. I might go for something a bit meatier this time, seeing as I'm no longer scared of OS 😂

I've made the change you requested - let me know if there's anything else you want me to do for the PR. Otherwise, I'll look forward to this PR being merged 🙌