Dashes and colons #191

adiabatic · 2018-01-22T22:13:04Z

This is a preliminary PR. I'm not totally sure it's the one I want, but I wanted to get feedback on it, mostly on the tests. I'd be happy to squash my commits into a single commit once it's all ready to go.

There aren't any specific tests to ensure that lines like "@food:molecular-gastronomy: ate a giant hamburger that was actually 100% jelly" don't parse the tag as "food:molecular-gastronomy:". I'll add those tests.

What's gnawing at the back of my mind is that I changed the tags on people in the sample data, and this impacts what the tests actually test. Before, tests ensured that going from 1 tag to 0 was OK. Now, tests ensure that going from 2 tags to 1 work as expected. We could add more people to the sample data and test both 2 to 1 and 1 to 0, but I'd have trouble choosing suitable sample friends in keeping with the preexisting theme you've got going. I could bring in Marie's husband Pierre, but I wouldn't know who else fits the theme you've got going.

If I change the preexisting tags like "@science" to something like "@science:indoor" (IIRC neither Curie nor Hopper did fieldwork like John Muir) and update the tests to match, would that give us the coverage we want?

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.
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!

coveralls · 2018-01-22T22:27:50Z

Coverage remained the same at 97.95% when pulling 9afed29 on adiabatic:dashes-and-colons into d53d25a on JacobEvelyn:master.

JacobEvelyn · 2018-01-23T00:05:39Z

Hey @adiabatic! Thanks for working on this (I know jumping into a foreign codebase isn't easy!) and for all of your thoughtfulness in how to incorporate the change well and have good test coverage.

In general I think your code looks good!

Regarding the test coverage you mention, I agree that it's important to test the 1-to-0 tags case (and would probably be good to test that with a tag that has some characters in it), which I think we can do by adding more people to the friends.md example file (I wasn't really following any pattern other than trying to put people who I think no one would look at the project and be offended by... someone like Nelson Mandela or Mahatma Gandhi or Rosa Parks, for instance, would be totally fine to add).

The other way to test this would be to define a custom let(:content) for the tests, as we do in some tests (example). Either way is fine by me.

As you mentioned, I do think it would be good to have tests to make sure that tags don't start or end with punctuation, and don't have multiple dashes or colons in a row (unless you can think of a reason why that would be a good feature to have?).

Another thing that should probably be tested is that we can use dashes and colons in tags on the command line without Bash or other shells doing weird interpretations of them (I'm happy to explain more if that doesn't make sense). For instance, the initial implementation of tags used # instead of @, which quickly got annoying because you'd do something like:

$ friends add activity Went to the grocery store with Marie #shopping
Activity added: "2018-01-22: Went to the grocery store with Marie Curie"

(where #shopping was interpreted as a shell comment and ignored).

One small note on the code you have: I think your regex might have a bug in it. Using Rubular it looks like it doesn't correctly find @a:b-c. After a bit of playing around I came up with what I believe is a cleaner alternative (though there might be some use case I'm not thinking of so please double-check my reasoning!):

@\p{Alpha}([:-]?\p{Alnum})*

Basically: an alphanumeric character, followed by 0 or more sets of "optional punctuation followed by alphanumeric character." This seems to work for everything I can throw at it, and guarantees that tags start and end with an alphanumeric.

So one last thing this has made me think about is whether it makes sense for colons in tags to have some amount of meaning. Like @science:chemistry or @science:particle-physics would be interpreted to be both the tags as-is and the @science "parent" tag. So you could do things like $ friends list activities --tagged science and it would find any activity with either a @science tag or a @science:* tag, if that makes sense. Another example:

$ friends list activities --tagged games:caverna
2018-01-22: Played games with **Marie Curie**! @games:caverna
$ friends list activities --tagged games
2018-01-22: Played games with **Marie Curie**! @games:caverna
2018-01-17: Played games with **George Washington Carver**! @games:settlers-of-catan

If you think that makes sense to do, we could talk about:

whether it makes sense to have that "hierarchy" be multi-level, like @games:board-games being a "parent" of @games:board-games:caverna.
whether it makes sense to incorporate that functionality into this change or a separate PR.

Thoughts?

science · 2018-01-23T18:20:30Z

Hi - sorry to drop in on you. I've been ignoring these messages for awhile, but you guys keep accidentally tagging me in your PRs and issues! :)

I'm @science - I'm not actually sure if "\" backslashes will escape the @ references in your code citations.. (It doesn't look like it in this post: @science). Possibly @science would work (backtick escaping). Anyway - not a big deal but I wanted to mention you were accidentally tagging me repeatedly.

JacobEvelyn · 2018-01-23T18:23:22Z

So sorry for all the noise, @science! We'll try to be more careful about backticks from now on! 😬

adiabatic · 2018-01-24T00:53:10Z

I agree that it's important to test the 1-to-0 tags case

I'll restore the tags for the preexisting people and add in a couple more regulars so we can test both 1->0 and 2->1 cases. Stan and Norm sound like good candidates to me.

Another thing that should probably be tested is that we can use dashes and colons in tags on the command line without Bash or other shells doing weird interpretations of them

Oh, that. Yeah. I do some journaling with Day One, and trying to remember what I had to escape gave me a headache. Eventually I gave up and just started typing dayone2 -j journal new at the command line and then started typing stuff into stdin, ending with a newline and ^D.

Theoretically friends could do that too, but someone would have to think about how to handle entries with more than one line in them. Probably not too difficult, but a different PR for this would be best.

Colons and hyphens don't seem like problems for bash because I've performed commands like scp new-foo.txt example.com:, but I haven't gotten into any of the more exotic shells like zsh or fish. My opinion is that trying to predict and avoid all the special characters used by other shells is a rat hole that we don't want to go down.

I think your regex might have a bug in it.

Ooh, good catch. The thing is, @1 is a tag under the old regex of

@\p{Alnum}+

but isn't with

@\p{Alpha}([:-]?\p{Alnum})*

In the off chance that some user somewhere is already using tags that start with a number, I think

@\p{Alnum}([:-]?\p{Alnum})*

is the better option.

[tag hierarchies with colons]

I'd very much like this too, but it's too much work for me right now and is best shoved off to another PR to be done at a later date. I'll happily tag things with coloned hierarchies, but I probably won't be needing filtering facilities like this for years.

whether it makes sense to have that "hierarchy" be multi-level, like @‌games:board-games being a "parent" of @‌games:board-games:caverna.

That makes complete sense to me.

Also, let's see if this auto-links: @‌JacobEvelyn. I put a ZWNJ between the at-sign and the username, so that should be a way to not accidentally tag people when we type out tags.

In the next PR: Updated, documented TAG_REGEX and tests that don't clobber the things we were testing previously.

JacobEvelyn · 2018-01-24T14:03:15Z

Hey @adiabatic! Thanks for all the thoughtful discussion.

I'll restore the tags for the preexisting people and add in a couple more regulars so we can test both 1->0 and 2->1 cases.

Sounds perfect.

Stan and Norm sound like good candidates to me.

Agreed!

Theoretically friends could do that too, but someone would have to think about how to handle entries with more than one line in them. Probably not too difficult, but a different PR for this would be best.

I'm not totally sure I follow. For avoiding shell interpretation of special characters, what you describe with dayone2 seems pretty similar to using friends add activity with the interactive prompt option. Is the difference you're talking about that you want newlines in your activities?

Colons and hyphens don't seem like problems for bash

They seem to work for me in zsh too, so I think that's good enough.

In the off chance that some user somewhere is already using tags that start with a number

Whoops, yes! I had been playing around with Alpha vs. Alnum, realized the same conclusion as you, but forgot to change it back. Good catch!

I'd very much like this too, but it's too much work for me right now and is best shoved off to another PR to be done at a later date.

Agreed that this should be a separate PR. I've made #192 for tracking that feature. Thanks for the feedback on that!

Lastly, thanks as well for (a) teaching me you can add titles to Markdown links here (yep, I noticed that 😄), and (b) informing me that Agricola has a successor!

Let me know when you've pushed changes to the PR and I'll take a look!

JacobEvelyn · 2018-02-01T00:10:56Z

Hey @adiabatic! Anything I can do to help you with this?

adiabatic · 2018-02-01T00:15:50Z

Thanks, but not really. Just been swamped with other things. I plan on finishing it up in a week or two.

adiabatic · 2018-02-01T07:53:19Z

what you describe with dayone2 seems pretty similar to using friends add activity with the interactive prompt option.

Oops. Didn't notice that option.

I'm having trouble getting Git to do what I want. Some of my pushes to the dashes-and-colons branch might be buggy, including the most recent one (dc2015c).

adiabatic · 2018-02-01T08:07:40Z

7c85dbe is a revision actually worth discussing.

JacobEvelyn · 2018-02-01T14:10:34Z

@adiabatic just skimmed it and the code looks pretty good to me! When you said "I plan on finishing it up in a week or two" does that mean there's something more you planned to add, or is this potentially good to go? (Seems fine to me but I want to make sure I'm not missing something.)

adiabatic · 2018-02-01T22:42:38Z

does that mean there's something more you planned to add, or is this potentially good to go?

I think I'm 95% of the way there, but there's one more thing I'd like to test.

Remember how we said "we don't want "@‌food: ate an eight-pound hamburger with Stan" to parse the tag as "‌food:", but rather "‌food"? Where should I put a "run friends add activity … and ensure the output of friends list tags doesn't have any colons at the end" test? test/commands/add/activity_spec.rb, tag_spec.rb, or someplace else entirely?

JacobEvelyn · 2018-02-03T21:04:46Z

Good question @adiabatic, and sorry for the slow response! Agreed that we should have that test, but I think there are actually two parts to it. One part is the "does this get interpreted correctly when we call friends list tags?" question (which I think belongs in list/tags_spec.rb), but the other is the question of "does this get colored correctly when we call friends list activities and print the colored output?" (which I'm realizing is something that isn't tested anywhere so I don't think we should add it here—I can add that in a later PR after thinking about it more).

So I guess that's a roundabout way of saying that I think testing it in list/tags_spec.rb is the way to go! In that file you can use the same trick of redefining let(:content) that I mentioned in my first comment for that test so that the markdown file includes some tricky cases like

2018-02-03: @food: I emailed goodfood@example.com and got great recipes to try out @cooking:culinary-arts!

Does that make sense?

adiabatic · 2018-02-04T03:58:34Z

No worries about the slow pace. I'm in no rush.

I guess that makes for three things that deserve testing, AFAICT:

Does the tag get interpreted without a trailing colon if friends add activity @‌science:indoors:agronomy: Norm and George scored a tour of Atlantis' hydroponics gardens through ilikeitdownhere@example.org and they took me along. is ran from the command line?
Does the tag get interpreted correctly when friends list tags is called?
Does the tag get interpreted correctly (i.e. is it colored correctly) in friends list activities?

Shouldn't we have tricky cases in both CONTENT and SCRAMBLED_CONTENT, and not just in a couple tests' :content redefinition, though?

JacobEvelyn · 2018-02-04T13:00:19Z

Thanks for the thoughts. Here's what I'm thinking:

Does the tag get interpreted without a trailing colon if friends add activity @‌science:indoors:agronomy: Norm and George scored a tour of Atlantis' hydroponics gardens through ilikeitdownhere@example.org and they took me along. is ran from the command line?

I thought we needed a test for this at first, but now I'm realizing that when you add an activity or note there really isn't any "interpretation" of the tag that happens, so I don't think there would be anything to test here. (The line that gets added to friends.md would basically just be 2018-02-04: @‌science:indoors:agronomy: **Norman Borlaug** and ..., which doesn't really indicate how the tag was interpreted.)

Does the tag get interpreted correctly when friends list tags is called?

Yep, we need to test this. I'd also vote for putting both colons and dashes in our tag if possible.

Does the tag get interpreted correctly (i.e. is it colored correctly) in friends list activities?

This is what I was trying to say around this being good to test but since we apparently don't have any tests for any of the coloring (i.e. of friends or locations) I think we should save this for another PR and not worry about lumping it in to this one.

Shouldn't we have tricky cases in both CONTENT and SCRAMBLED_CONTENT, and not just in a couple tests' :content redefinition, though?

I'm happy either way. As long as we're testing the behavior I don't think it matters if it's in just one test or if the behavior is happening in a bunch via CONTENT/SCRAMBLED_CONTENT. Whatever you prefer here!

Also, one last thing I'm thinking of: we should update the README.md in this PR just to mention that tags can have dashes and colons and add an example or two.

Does all of that make sense?

adiabatic · 2018-02-06T22:42:41Z

Makes sense. I'll update the README if everything I've done here so far looks OK, or is at least on the right track.

JacobEvelyn

@adiabatic this looks really good to me! Just a few tiny comments (let me know if you can't see them—I've never used GitHub's PR review system before and could be doing something wrong 😬) but this is pretty much good to go.

The only other thing is adding one or two examples of this to the README. Would you be willing to add that into your next commit?

JacobEvelyn · 2018-02-13T04:15:57Z

lib/friends.rb

@@ -3,5 +3,5 @@
 require "friends/version"

 module Friends
-  TAG_REGEX = /@\p{Alnum}+/
+  TAG_REGEX = /(?<!\p{Alnum})@\p{Alnum}(?:[:-]?\p{Alnum})*/


Ah, smart move with the negative lookbehind at the beginning. Technically this could inadvertently catch some false positives like very weird emails (example.@example.com) but that's so edge-casey I don't think we should worry about it. It's definitely better than before!

JacobEvelyn · 2018-02-13T04:18:56Z

test/commands/add/tag_spec.rb

      end
    end

    describe "when friend already has tags" do
      let(:friend_name) { "Marie" }
+      let(:tag) { "school:ecole-normale-superieure" }


Very minor, but would you mind adding the French accents here? (school:école-normale-supérieure) I think Ruby/the shell/the regex should all handle those characters fine but it would be nice to have it in a test.

Should we do that, though? If we encourage non-7-bit-ASCII tags, we ought to do some sort of normalization. I had a look at the wikipedia page on the subject, looked at NFD, NFC, NFKD, and NFKC and the best method I have at hand for picking the best normalization form for our purposes is eenie-meenie-miney-moe.

I don't feel strongly here so I'll defer to you. Thanks for doing so much digging on this!

JacobEvelyn · 2018-02-13T04:19:16Z

test/commands/edit_spec.rb

@@ -77,6 +77,7 @@
    let(:cleaned_edited_content) do
      <<-EXPECTED_CONTENT
 ### Activities:
+- 2018-02-06: @science:indoors:agronomy-with-hydroponics: **Norman Borlaug** and **George Washington Carver** scored a tour of _Atlantis_' hydroponics gardens through wetplants@example.org and they took me along.


Fun example! 😄

JacobEvelyn · 2018-02-13T04:20:36Z

test/commands/remove/tag_spec.rb

@@ -41,13 +41,24 @@
      let(:tag) { "science" }

      it "removes tag from friend" do
-        line_changed "- Marie Curie [Atlantis] @science", "- Marie Curie [Atlantis]"
+        line_changed "- Marie Curie [Atlantis] @science",


Is this just a stylistic change or am I missing something subtle here?

Looks stylistic to me. I started breaking lines defensively to avoid rubocop's warnings.

Plus, it's easier to see what's changed if everything lines up vertically.

Works for me! 👍

JacobEvelyn · 2018-02-13T04:21:34Z

test/commands/set/location_spec.rb

@@ -35,7 +35,8 @@
      end

      it "updates existing location" do
-        line_changed "- Marie Curie [Atlantis] @science", "- Marie Curie [Paris] @science"
+        line_changed "- Marie Curie [Atlantis] @science",


Same question here on what's changed.

Still stylistic.

JacobEvelyn · 2018-02-13T04:25:09Z

lib/friends.rb

@@ -3,5 +3,5 @@
 require "friends/version"

 module Friends
-  TAG_REGEX = /@\p{Alnum}+/
+  TAG_REGEX = /(?<!\p{Alnum})@\p{Alnum}(?:[:-]?\p{Alnum})*/


One small thing I'm wondering here though... is it weird that tags match case-sensitively? Should @games:caverna and @games:Caverna really be different?

(To be clear, I had been wondering this already—it's nothing to do with your changes—and I'm happy to make that change in a separate PR. More just curious for your thoughts.)

Exact-byte-match (bolstered by a strong convention of all-lowercase tags) is certainly the easiest thing to do, and more complicated things aren't a Pareto improvement (an improvement that makes at least one person better off and nobody worse off). I could imagine someone using @‌nice for "this was a nice thing that happened" and @‌Nice for "something concerning this one city in the southeast of France", or something else that differs only by capitalization.

I'd say "don't do any of this until someone asks for it", but it's worth thinking about these things as a group, although implementing one doesn't necessarily mean you're obliged to implement them all:

case-insensitive tag matching (but case-preserving, like HFS+, APFS, and NTFS file names)

ensuring that all strings are folded into one particular normalization form (Ruby might do this properly itself; I don't know.)

diacritic-insensitive matching, at least for some locales

Nobody's asking for any of this AFAIK, so I'd personally hesitate to implement any of these features if friends were my project.

Wow, thanks for such a thoughtful response. Yeah I guess I buy the argument that it's fine to leave these for now until someone needs them to behave otherwise.

JacobEvelyn · 2018-02-13T04:28:45Z

Thanks so much, @adiabatic! Sorry for the delay; I've had an unexpectedly busy week but I know firsthand how annoying it can be to wait for a maintainer to look at your code so sorry again! I left you a "PR review" but let me know if you can't see my comments or have any questions. In general this looks really good! Just one or two small tweaks and then the README!

Thanks for all the hard work and patience!

JacobEvelyn · 2018-02-20T15:14:42Z

@adiabatic would you prefer that I clean this up and do the finishing touches so we can get this out? Up to you!

adiabatic · 2018-02-21T10:40:01Z

Sorry 'bout the eight-day pause. I think it's good to go.

JacobEvelyn · 2018-02-23T12:05:48Z

Awesome, this all looks fantastic. (And no worries about the delay.) Thanks so much for your thoughtfulness with everything. Would you be down to squash all of this into a single commit with some reasonable commit message? Something like the following is fine, but feel free to write more if you want!

Add support for dashes and colons to tags

Closes #186

(The Closes #186 is important to get the sometimes-finicky autogenerated changelog to work correctly.)

I'm also more than happy to do the squash (you'd still be listed as the author in git and GitHub) but am mindful of the fact that that might make it harder to follow your contribution across GitHub repos since I can't push to your fork. Just let me know!

adiabatic · 2018-02-24T00:29:40Z

I could use the practice. I'll squash it myself.

Closes #186

JacobEvelyn · 2018-02-24T16:26:03Z

Thanks so much for all the hard work, @adiabatic! This is an awesome improvement. This feature is now released in v0.37 (friends update to get it) and you're officially a contributor! (And I hope you'll contribute more in the future!)

You rock.

JacobEvelyn mentioned this pull request Jan 24, 2018

Allow tags to be hierarchical #192

Open

JacobEvelyn reviewed Feb 13, 2018

View reviewed changes

Support dashes and colons in tags

9afed29

Closes #186

JacobEvelyn merged commit 7688d7a into JacobEvelyn:master Feb 24, 2018

adiabatic deleted the dashes-and-colons branch February 28, 2018 08:40

Dashes and colons #191

Dashes and colons #191

Conversation

adiabatic commented Jan 22, 2018 • edited by JacobEvelyn Loading

coveralls commented Jan 22, 2018 • edited Loading

JacobEvelyn commented Jan 23, 2018

science commented Jan 23, 2018

JacobEvelyn commented Jan 23, 2018

adiabatic commented Jan 24, 2018

JacobEvelyn commented Jan 24, 2018

JacobEvelyn commented Feb 1, 2018

adiabatic commented Feb 1, 2018

adiabatic commented Feb 1, 2018

adiabatic commented Feb 1, 2018

JacobEvelyn commented Feb 1, 2018

adiabatic commented Feb 1, 2018

JacobEvelyn commented Feb 3, 2018 • edited Loading

adiabatic commented Feb 4, 2018 • edited Loading

JacobEvelyn commented Feb 4, 2018

adiabatic commented Feb 6, 2018

JacobEvelyn left a comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

JacobEvelyn commented Feb 13, 2018

JacobEvelyn commented Feb 20, 2018

adiabatic commented Feb 21, 2018

JacobEvelyn commented Feb 23, 2018

adiabatic commented Feb 24, 2018

JacobEvelyn commented Feb 24, 2018

adiabatic commented Jan 22, 2018 •

edited by JacobEvelyn

Loading

coveralls commented Jan 22, 2018 •

edited

Loading

JacobEvelyn commented Feb 3, 2018 •

edited

Loading

adiabatic commented Feb 4, 2018 •

edited

Loading