Problem Types, Hiding Individual Errors, Accurate Word Highlighting, TODO Tasks #51
Conversation
individual warnings), warning numbers on problems and better identification of problem types (warning, info, error).
character index when scanning a line for tabs. This caused incorrect positioning of the cursor for some errors
offset -- fixed. Implemented better highlighting of a problem's related token. Adjusted problem formatting slightly.
|
I've fixed a bug in the way tabs were being accounted for in the fixPosition function. I also adjusted some of my earlier code that wasn't taking into account the extra annotations line in some places. With all of the bug fixes out of the way, I've implemented much more comprehensive highlighting of the related term for problems. This means you get a nice little squiggly yellow or red line under the relevant term in your code. This function in MarkerHandler.java seems wrong to me: private boolean isValidCharacter( int line, int character ) { Given that characters are an index running from 0 to lineLength - 1, it would seem to me that the function should be: private boolean isValidCharacter( int line, int character ) { Right now, it allows a marker to be on the nth+1 char of a line, which I think puts it on the next line though I haven't confirmed this. I'd like your input. |
settings, start of support for grabbing TODO tasks from the central store.
|
My fork now recognises TODO tasks, but only the default three (TODO, XXX, FIXME). Couldn't figure out how to read the prefs from the central store, so I'm leaving it for now. Here's a quick visual summary of the changes: |
|
I figured out how to read central preferences for TODO tasks. It's now reading them from the WST preferences store. This could easily be changed to our own, or another, more suitable store. On my machine, I can get to this by right-clicking on the project, choosing Properties and then Task Tags on the left. |
|
Thanks a lot for your contribution, it's very appreciated. However, it may take a couple of weeks until I find the time to look at it. |
|
You're very welcome. This is an excellent project. It was just missing a few key features that I and others needed. Why not take the time to (hopefully) improve it? In hindsight, it might have been better to offer you separate pull requests for each feature but oh well. Let me know if anything doesn't make sense and I will explain it. |
|
Wow, amazing contribution, @gurok, thank you very much! I was thinking about to implement the error/warning differentiation myself - if it fails the build, it should displayed as an error ;) |
|
@gurok Could you please update your fork to the latest 0.9.8 changes. |
|
Sorry for the delay. I'm really busy right now. My code is so out-of-date now, merging would be nightmare. You're free to cherry pick from it as you see fit. EPL? Public domain, whatever you want :D. I really don't mind. That patch for the bug in fixPosition was actually pretty urgent. I'm surprised it hasn't caused more exceptions. |
|
This pull request addresses many different issues, which makes it a bit hard to sort out. Since most issues require some more discussion, it's generally better to open separate issues or pull requests for separate concerns. Discussing different topics in one github issue would become very confusing. BugsfixPosition was using the "tabbed in" index instead of the actual (gurok@7f720c9)I needed a while to see it, but you're completely right. I cherry-picked your commit with 9ca5109 and refactored the method again in 6de5efc. Thanks! Off-by-one in MarkerHandler (#51 (comment))Good catch! Fixed with 5a452b2. Thank you! FeaturesSupport for error markersI've included a slightly modified version of @ben8p's change (see #45) Support for default annotations for JS filesSorry, I didn't quite get it. Some more explanation would be helpful. In any case, this feature should be discussed in a separate issue. This would also allow others to state their interest. Support for ranges (squiggly lines)As far as I understand, you're guessing the ranges from tokens extracted from the error message. That probably opens the door for a lot of follow-up issues (ranges that do not fit, missing ranges, etc.) and probably also differences from one jshint version to another. I'd prefer to ask jshint to provide ranges. If you disagree, again please open a new issue for this feature. Support for TODO markersI doubt that TODO markers should be the responsibility of a jshint integration. For me, this sounds like a different plug-in. However, if there's broad interest in this feature, let's discuss this in a new issue. |
|
Hey, no problem. These all sound really reasonable. Some of the stuff was a bit half cooked to be honest. I added default annotations because I was sick of adding annotations to every file to turn off a warning. There's probably a better way to do it. Maybe a jshint.rc or some similar file where you can specify such things, but I was working with what JSHint Eclipse was actively parsing. Yeah, this was pretty bad, honestly. The ranges stuff wasn't so bad. I don't think my code was guessing, merely searching after the cursor for the first parameter mentioned by the error. If there was none, there was no highlighting. Yeah, I suppose it's better to use what JSHint provides, but what it provides is really incomplete right now. Most of the other builders accept TODO markers. I don't think it's the role of another plugin unless you expect folks to have multiple builders for a JS project. If this is the only builder, it should really handle TODO. But, that said, there's probably a more standard way of doing it. I'm just glad the major things got in. If you committed ben8p's stuff, I hope you modified it to allow for all message types (not just error/warning). I seem to remember his wasn't so flexible and I ended up making it a bit spiffier. |
|
Hi @gurok, .jshintrc files are already supported, that should help to turn off certain warnings on project level. I said your ranges code guesses mostly because I've seen that it relies on the TODO markers: Paul already opened a request, let's discuss there. Currently only errors and warnings are supported, but it's not a big deal to add info once they're needed - design for the moment! |
Hi there,
I've copied in some changes to support multiple problem types from a fork by ben8p. I improved upon his code so that it now identifies all 3 problem types (warning, info and error). I've also added support for setting a default annotation for all JS files so that you can disable a particular warning number (e.g. W078) through the project properties. Finally, I appended the error number to the error message before it's put in the Problems tab. Makes it easier to identify an error and disable it.
I haven't paid much attention to testing, so you might want to look it over. I basically added extra params where necessary to stop the tests complaining and used sensible defaults, but you might like to add more tests for the new params if you accept this :)