Skip to content
This repository has been archived by the owner on Sep 6, 2021. It is now read-only.

duplicateText amended to handle end-of-file scenarios, adding new line; ... #1179

Merged
merged 3 commits into from
Jul 13, 2012

Conversation

glortho
Copy link

@glortho glortho commented Jun 27, 2012

...testing thereof; Replacement for previous pull request. issue #926

@ghost ghost assigned peterflynn Jun 27, 2012
@peterflynn
Copy link
Member

I'll review this in the next 1-2 days.

@pthiess
Copy link
Contributor

pthiess commented Jun 29, 2012

@jedverity: Before we can accept this into the project, we need you to sign the Contributor License Agreement -- http://dev.brackets.io/brackets-contributor-license-agreement.html

@glortho
Copy link
Author

glortho commented Jun 29, 2012

I did sign that a couple of days ago. Do you not have a record of it? Let me know and I'll sign it again.

On Thursday, June 28, 2012 at 11:53 PM, Peter Thiess wrote:

@jedverity: Before we can accept this into the project, we need you to sign the Contributor License Agreement -- http://dev.brackets.io/brackets-contributor-license-agreement.html


Reply to this email directly or view it on GitHub:
#1179 (comment)

if (!hasSelection) {
sel.start.ch = 0;
sel.end = {line: sel.start.line + 1, ch: 0};
delimiter = typeof doc.getLine(sel.end.line) == "undefined" ? "\n" : delimiter;
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

  1. It'd be cleaner to check sel.end.line against editor.totalLines() here. (Also, note: to pass JSLint, use === instead of == for comparisons).
  2. Since that doesn't use doc, you could leave its var declaration down below.
  3. The use of ternary operator feels a little weird here since it's a no-op on one branch. I think it'd read better if this was just a standard if statement.

Copy link
Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I think you mean editor.lineCount() but let me know if I'm missing something!

@peterflynn
Copy link
Member

Great job @jedverity -- thanks for the solid fix! Apologies for the long delay in reviewing -- vacation got the better of me. Feel free to ping me on IRC if you want to discuss any of the comments (or just post back here of course).

I've verified your 'Contributor License Agreement' record, so no worries there.

@glortho
Copy link
Author

glortho commented Jul 10, 2012

Thank you, @peterflynn ! I've pushed the changes as you requested (though not sure why it's showing up as my alternative username).

@peterflynn
Copy link
Member

Two more changes needed and then I think we're good to merge! Thanks again!

Re "totalLines()" vs. lineCount(): yep, you're right -- my bad.

Re declaring all vars at the top: wouldn't be the first time it's been debated here :-) Officially the Brackets coding guide is neutral (either style is ok), but personally I have a pretty strong preference for declaring vars where they're initialized -- only in a project like Brackets where passing JSLint is a strict requirement. Combining JSLint with lower-down var declarations automatically catches the class of bugs where you use a var before it's been initialized (which JSLint is not smart enough to do if you move the var declaration earlier). The more bugs you find right when you hit Cmd+S, the less time spent wading around the debugger or the GitHub bug list later ;-)

@peterflynn
Copy link
Member

Great -- thanks! Merging now...

peterflynn added a commit that referenced this pull request Jul 13, 2012
Fix bug #926: amend duplicateText() to handle end-of-file scenarios, with unit test
@peterflynn peterflynn merged commit a2a5c04 into adobe:master Jul 13, 2012
Sign up for free to subscribe to this conversation on GitHub. Already have an account? Sign in.
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

3 participants