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

Include "Help make this better" text on track topic pages #3475

Merged
merged 11 commits into from May 17, 2017

Conversation

@nholden
Copy link

@nholden nholden commented Apr 22, 2017

#3355 pointed out that the instructions for filing an issue or submitting a patch were not showing up on language track topic pages. This adds that text to the bottom of those pages.

else
fallback_topic_content(topic_name)
end
track_docs.each_pair do |topic_key, topic_content|

This comment has been minimized.

@nholden

nholden Apr 22, 2017
Author

Since topic_key is a symbol, I thought this was a better name than topic_name, which sounds like a string to me.

This comment has been minimized.

@Insti

Insti Apr 22, 2017

Note for next time: It would have been nice to make this name change a separate commit.

assert topic_doc.include? "Default topic content"
assert !topic_doc.include?("Help us explain this better!")
assert !topic_doc.include?("File a GitHub issue at https://github.com/exercism/test_track")
assert !topic_doc.include?("https://github.com/exercism/test_track/blob/master/docs/TEST_TOPIC.md")

This comment has been minimized.

@nholden

nholden Apr 22, 2017
Author

The fallback content (example) already includes information about how to submit a patch with track-specific content, so including the "better" content here would be redundant.

# Enhances the track-specific documentation, adding
# defaults where missing and embellishments that encourage
# people to submit improvements.
class Track

This comment has been minimized.

@nholden

nholden Apr 22, 2017
Author

Now that we use the Trackler gem to grab track-specific content, we don't use this class. Instead, we have a Track presenter that essentially wraps the Trackler track.

topic_doc = @track.docs["test_topic"]

assert topic_doc.include? "Default topic content"
assert !topic_doc.include?("Help us explain this better!")

This comment has been minimized.

@Insti

Insti Apr 22, 2017

use refute instead of assert !

This comment has been minimized.

@nholden

nholden Apr 22, 2017
Author

I didn't know about refute -- good call! I handled that in 2adfd9d.

[topic_content.strip, better_content(topic_key)].join("\n")
else
fallback_topic_content(topic_key)
end

This comment has been minimized.

@Insti

Insti Apr 22, 2017

I think this whole section could be tidied up a bit.

This comment has been minimized.

@nholden

nholden Apr 23, 2017
Author

I totally agree. I took a stab at breaking the docs method apart in 1a44a1e. What do you think?

@@ -40,6 +40,14 @@ def trackler_track
Trackler.tracks[@track_id]
end

def better_content(topic_key)

This comment has been minimized.

@Insti

Insti Apr 22, 2017

This name is a bit confusing to me.

Why is it better_content?

This comment has been minimized.

@nholden

nholden Apr 22, 2017
Author

You're right, that's a confusing name. I renamed it contributing_instructions in a663a3d

@@ -7,20 +7,34 @@ class PresentersTrackTest < Minitest::Test
def setup
@track = ExercismWeb::Presenters::Track.new("test_track")
@trackler_track = Object.new

@track.stubs(:trackler_track).returns(@trackler_track)
@track.stubs(:fallback_topic_content).returns("Default topic content")

This comment has been minimized.

@Insti

Insti Apr 22, 2017

I realise this is not your fault, but I really don't like stubbing the object under test.
tracker_track could be tested by stubbing Trackler instead.
Is fallback_topic_content ever tested?

This comment has been minimized.

@nholden

nholden Apr 23, 2017
Author

Good points. I reworked the tests in 827a210.

@Insti
Insti approved these changes Apr 22, 2017
Copy link

@Insti Insti left a comment

The changes that @nholden has made are all reasonable.
My nitpicks appear to all be with the pre-exisiting code.
That shouldn't be a blocker for merging.

Nick Holden added 5 commits Apr 22, 2017
Issue #3355 pointed out that the instructions for filing an issue or
submitting a patch were not showing up on language track topic pages.
This adds that text to the bottom of those pages.
We're no longer using X::Docs::Track to display track-specific
documentation. Instead, we have a Track presenter that wraps data
returned by the Trackler gem.
@nholden nholden force-pushed the nholden:better-text branch from a0a6ba2 to 1a44a1e Apr 23, 2017
@nholden
Copy link
Author

@nholden nholden commented Apr 23, 2017

@Insti Thanks for all your helpful comments! I went ahead and tried to address them here.

If you'd prefer, I'm also happy to let someone merge what you approved and open up another PR for refactoring. Let me know what you think!

fallback_topic_content(topic_name)
end
trackler_topics.each_with_object(OpenStruct.new) do |topic_key, result|
result[topic_key] = topic_content(topic_key)

This comment has been minimized.

@Insti

Insti Apr 23, 2017

Excellent

@Insti
Insti approved these changes Apr 23, 2017
Copy link

@Insti Insti left a comment

I really like everything you've done here.

I have a few more minor things that could be improved, feel free to tell me you don't want to do them.

Trackler.stubs(:tracks).returns(trackler_tracks)
trackler_tracks.stubs(:[]).returns(@trackler_track)
@trackler_track.stubs(:repository).returns("https://github.com/exercism/test_track")
@trackler_track.stubs(:doc_format).returns("md")

This comment has been minimized.

@Insti

Insti Apr 23, 2017

Minor thing: I'd put these 2 lines (15-16) directly under line 11 so the @trackler_track is fully initialised before you try to use it anywhere.

@trackler_track.stubs(:docs).returns(trackler_docs)
assert_equal @track.docs["test_topic"], "Track-specific content from Trackler"
@trackler_track.stubs(:docs).returns(OpenStruct.new(about: "Track-specific content from Trackler"))
topic_doc = @track.docs["about"]

This comment has been minimized.

@Insti

Insti Apr 23, 2017

Don't hide the thing you're testing.

inline and remove @track completely.

assert topic_doc.include? "We're missing a short introduction about the language."
refute topic_doc.include? "Help us explain this better!"
refute topic_doc.include? "File a GitHub issue at https://github.com/exercism/test_track"
refute topic_doc.include? "https://github.com/exercism/test_track/blob/master/docs/TEST_TOPIC.md"

This comment has been minimized.

@Insti

Insti Apr 23, 2017

Are these refute tests necessary?

This comment has been minimized.

@nholden

nholden Apr 23, 2017
Author

My initial thinking with the refute tests is that I wanted to show that we're only adding the "Help make this better" text to topic pages where we have track-specific content. They were useful as I was writing the code originally, but I don't think they're totally necessary now, so I removed them.

This comment has been minimized.

@Insti

Insti Apr 23, 2017

Good decision.
Refactoring the tests after you've written the code is an important and often neglected step in TDD.

@nholden
Copy link
Author

@nholden nholden commented Apr 23, 2017

Thanks, @Insti! I addressed your latest batch of feedback in 3afedaa, and I think the tests are looking a lot clearer.

@Insti
Copy link

@Insti Insti commented Apr 23, 2017

Great, thanks for all your work on this @nholden, I think this looks ready to go.
I'll let someone else give it a quick look over and hit the merge button in case I've missed anything.

@Insti
Insti approved these changes Apr 23, 2017
@Insti
Copy link

@Insti Insti commented Apr 23, 2017

I also really liked the atomicity of your commits, it was a pleasure to review them. ❤️

def contributing_instructions(topic_key)
File.
read("./x/docs/md/track/BETTER.md").
gsub('REPO', trackler_track.repository).

This comment has been minimized.

@Insti

Insti Apr 28, 2017

Is this fallback_topic_content('BETTER').gsub(....?

This comment has been minimized.

@nholden

nholden Apr 28, 2017
Author

That line only filled in REPO in the "help make us better" text at the bottom of track topic pages that we have content for. It doesn't replace REPO on the fallback pages when we don't have content for a track topic.

To address #3490, I made sure we were replacing REPO on our fallback pages, too, in d690e72.

This comment has been minimized.

@Insti

Insti Apr 28, 2017

This commit had not been merged yet, so I assume that this was a pre-existing bug?

This comment has been minimized.

@Insti

Insti Apr 28, 2017

This commit had not been merged yet, so I assume that this was a pre-existing bug?

Correct, no template replacements at all were being done on the fallback pages.
Confirmed by diffing back to master.

end

def content_with_trackler_data(filepath)
File.read(filepath).gsub('REPO', trackler_track.repository).gsub('EXT', trackler_track.doc_format)

This comment has been minimized.

@Insti

Insti Apr 28, 2017

Another round of "this is good, but what about":

The File.read has now been decoupled from the File.exist? I'd like to see these much closer to each other.

Then if the template replacer isn't reading the file AND doing the template replacements, it could take the already loaded data as an argument: track_specific_template_replacements(file_data)

I've chosen this name because I want to have an idea of a) something is being replaced, b) what that might be.

Another way to do this would be passing in the trackler track so we get a clue about what it might be template_replacements(file_data, trackler_track).

But it doesn't matter a whole lot in this case which solution you prefer.

Edit:

  1. For clarity.
  2. For realising track_specific_template_replacements is terribly ambiguous. Please use some other name.

This comment has been minimized.

@nholden

nholden Apr 29, 2017
Author

More good points!

In 33cdb68, I made sure that we were reading the File in the same method we check for its existence.

In 2b34498, I made some naming changes for clarity. I like how your suggestion of "template" signals content that has parts that still need to be filled in, so I latched on to that.

@@ -47,17 +47,17 @@ def topic_content(topic_key)
end

def contributing_instructions(topic_key)
content_with_trackler_data("./x/docs/md/track/BETTER.md").gsub('TOPIC', topic_key.to_s.upcase)
populate_template_with_trackler_data(File.read("./x/docs/md/track/BETTER.md")).gsub('TOPIC', topic_key.to_s.upcase)

This comment has been minimized.

@Insti

Insti Apr 29, 2017

Could this be made to use fallback_topic_content('BETTER') ?

This comment has been minimized.

@Insti

Insti Apr 29, 2017

I'm mostly worried about the fallback file path and file name format being defined in two places.

This comment has been minimized.

@nholden

nholden Apr 29, 2017
Author

We're including these contributing instructions intentionally on every track-specific topic page, so it's not really "fallback content." It just happens that the contributing instructions template and the fallback content templates live in the same directory and have the same file format.

It's a good point that we probably don't want to define those twice though. In 483e17f, I named the collection of the contributing instructions template and the fallback content templates "shared templates," because they'll show up on multiple track topic pages.

This comment has been minimized.

@Insti

Insti Apr 29, 2017

Thanks, I see.

I suspect this distinction is important. I wonder if there is a way to make it more clear in the code. The last few refactorings have been pushing the implementations closer together, but that may have been the wrong thing to be doing.

Do you have any suggestions?
("Don't worry so much about it" is valid 😉 )

This comment has been minimized.

@nholden

nholden Apr 30, 2017
Author

I'm leaning toward "don't worry so much about it," but I could totally be convinced otherwise.

The fallback content and the contributing instructions share a whole bunch of things in common -- in both cases, we're reading markdown files in the same directory and replacing some variables -- so I think it makes sense that their implementations share some things.

populate_template_with_trackler_data(File.read(filepath))
template_filepath = "./x/docs/md/track/#{topic_key.upcase}.md"
return '' unless File.exist?(template_filepath)
template = File.read(template_filepath)

This comment has been minimized.

@Insti

Insti Apr 29, 2017

I like this 👍

@@ -47,12 +47,12 @@ def topic_content(topic_key)
end

def contributing_instructions(topic_key)
template = File.read("./x/docs/md/track/BETTER.md")
template = File.read(shared_template_filepath("BETTER"))
populate_template_with_trackler_data(template).gsub('TOPIC', topic_key.to_s.upcase)
end

def fallback_topic_content(topic_key)

This comment has been minimized.

@Insti

Insti Apr 29, 2017

The signatures of these 2 methods look very similar.

def contributing_instructions(topic_key)
and
def fallback_topic_content(topic_key)

I wonder if keyword arguments could help us here by making explicit what topic_key is going to be used for.

def contributing_instructions(replacement_value: topic_key)
and
def fallback_topic_content(file_basename: topic_key)

What do you think?

This comment has been minimized.

@nholden

nholden Apr 30, 2017
Author

I'm a little torn. Do you think it's important that whatever's calling these methods knows how topic_key will be used for?

Here's the place where we're calling these methods now:

def topic_content(topic_key)
  if trackler_topic_content(topic_key).present?
    [trackler_topic_content(topic_key).strip, contributing_instructions(topic_key)].join("\n")
  else
    fallback_topic_content(topic_key)
  end
end

I think it's nice that topic_content doesn't need to know the implementation of contributing_instructions or fallback_topic_content. Instead, it passes along the topic_key and it gets back the relevant content.

This comment has been minimized.

@Insti

Insti May 1, 2017

You make a good point, I think I'd become so focused on those two methods that I lost track of the context.

@Insti
Insti approved these changes May 1, 2017
@kytrinyx
Copy link
Member

@kytrinyx kytrinyx commented May 17, 2017

This is great, @nholden, thanks so much! ❤️

And thanks for all the thorough review, @Insti 🌻

@kytrinyx kytrinyx merged commit 6abcdc5 into exercism:master May 17, 2017
2 checks passed
2 checks passed
continuous-integration/travis-ci/pr The Travis CI build passed
Details
coverage/coveralls Coverage increased (+0.3%) to 86.234%
Details
@nholden nholden deleted the nholden:better-text branch May 17, 2017
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Linked issues

Successfully merging this pull request may close these issues.

None yet

3 participants