Open valid url under cursor #4970
Conversation
Codecov ReportBase: 86.06% // Head: 86.06% // Decreases project coverage by
Additional details and impacted files@@ Coverage Diff @@
## master #4970 +/- ##
==========================================
- Coverage 86.06% 86.06% -0.01%
==========================================
Files 544 544
Lines 41444 41470 +26
Branches 6537 6541 +4
==========================================
+ Hits 35670 35691 +21
- Misses 5774 5779 +5
Flags with carried forward coverage won't be shown. Click here to find out more.
Help us with your feedback. Take ten seconds to tell us how you rate us. Have a feature suggestion? Share it here. ☔ View full report at Codecov. |
src/editor.js
Outdated
| /** | ||
| * Open valid url under cursor in another tab | ||
| **/ | ||
| this.jumptodef = function () { |
There was a problem hiding this comment.
Should we call it openLink instead? Jump to definition has usually a different meaning across
src/editor.js
Outdated
| * Finds link at defined {row} and {column} | ||
| * @returns {String} | ||
| **/ | ||
| this.findLinkAt = function (row,column) { |
There was a problem hiding this comment.
can we add a test for this function?
src/editor.js
Outdated
| this.findLinkAt = function (row,column) { | ||
| var line = this.session.getLine(row); | ||
| var wordParts = line.split(/((?:https?|ftp):\/\/[\S]+)/); | ||
| var delta = column; |
There was a problem hiding this comment.
can we rename delta to columnPosition instead to reflect the intent?
src/editor.js
Outdated
| if (delta < 0) delta = 0; | ||
| var curLength = 0, itLength = 0, match; | ||
| wordParts.forEach(function (item) { | ||
| itLength = curLength + item.length; |
There was a problem hiding this comment.
can we rename curLength to previousPosition and itLength to currentPosition?
| itLength = curLength + item.length; | ||
| if (delta >= curLength && delta <= itLength) { | ||
| if (item.match(/((?:https?|ftp):\/\/[\S]+)/)) { | ||
| match = item.replace(/[\s:.,'";}\]]+$/, ""); |
There was a problem hiding this comment.
should we add break here so we don't look at the rest of the line? it can help with large lines
|
@andrewnester Thank you for review! All should be fixed now. |
There was a problem hiding this comment.
Build fails with
/home/runner/work/ace/ace/src/editor_commands_test.js
Warning: 8:5 warning 'ace' is assigned a value but never used no-unused-vars
Warning: 9:5 warning 'EditSession' is assigned a value but never used no-unused-vars
Error: 546:6 error Unexpected trailing comma comma-dangle
✖ 3 problems (1 error, 2 warnings)
1 error and 0 warnings potentially fixable with the--fixoption.
Oh, sorry, haven't noticed this. Fixed now. |
Issue #, if available:
Description of changes:
By submitting this pull request, I confirm that you can use, modify, copy, and redistribute this contribution, under the terms of your choice.