[WIP] issue #622 Disable lint by default #663 #2

sgupta7857 · 2017-03-24T20:22:56Z

@humphd I have made some progress on this bug. After our conversation in class, I was able to add the code that kills the dependancy for .brackets.json file by having default preferences defined with in main.js [of extension]
and lint is disable now by default
Now further I will be implementing onchange/onclick functions that will allow me to interact with toggle button in thimble.

…o load in browser

sgupta7857 · 2017-03-24T20:31:04Z

Nope, it's wrong. The only reason it works is because .bracket.json is not present
I will try to fix it

humphd

Left some feedback. Let me know how you progress.

humphd · 2017-03-27T15:27:10Z

src/config.json

        "url": "https://github.com/mozilla/brackets.git",
        "branch": "",
        "SHA": ""
    },


You can do git checkout master src/config.json to get rid of this change.

humphd · 2017-03-27T15:27:26Z

src/extensions/default/Brackets-InteractiveLinter/main.js

+        var preferencesID = "interactive-linter";
+
+        var defaultPreferences = { 
+        delay: 500,


These need to get indented 4-spaces.

humphd · 2017-03-27T15:28:17Z

src/extensions/default/Brackets-InteractiveLinter/main.js

+        delay: 500,
+        enabled: false,
+        webworker: true,
+        javascript: "eslint" 


This is wrong. If you look above at .brackets.json you'll see that it's an array of strings.

humphd · 2017-03-27T15:28:35Z

src/extensions/default/Brackets-InteractiveLinter/main.js

+            _preferences.definePreferences("enabled", "boolean", defaultPreferences.disable);
+            _preferences.definePreferences("delay", "int", defaultPreferences.delay); 
+            _preferences.definePreferences("webworker", "boolean", defaultPreferences.webworker);
+            _preferences.definePreferences("javascript", "string", defaultPreferences.javascript); 


Also wrong here, you'll need to define an array of strings instead.

humphd · 2017-03-27T15:28:47Z

src/extensions/default/Brackets-InteractiveLinter/main.js

+            _preferences.definePreferences("webworker", "boolean", defaultPreferences.webworker);
+            _preferences.definePreferences("javascript", "string", defaultPreferences.javascript); 
+        }
+


Remove all this extra whitespace.

humphd · 2017-03-27T15:29:41Z

src/extensions/default/Brackets-InteractiveLinter/main.js

     */
    function appReady() {
+
+        loadPreferences();


Remove the blank live above this, move it below.

humphd

Looking good. I left some notes about fixes to make, and also a way to have us load less code when the extension is disabled on startup.

humphd · 2017-04-03T14:16:25Z

src/extensions/default/Brackets-InteractiveLinter/main.js

        linterManager      = require("linterManager"),
        pluginManager      = require("pluginManager");

+


Get rid of this empty line.

humphd · 2017-04-03T14:17:00Z

src/extensions/default/Brackets-InteractiveLinter/main.js

        pluginManager      = require("pluginManager");

+
+        var defaultPreferences = { 


This entire code block (line 25-34) is indented one tab stop too far. Make it line up with what's above and below it

humphd · 2017-04-03T14:30:43Z

src/extensions/default/Brackets-InteractiveLinter/main.js

            addGutter(currentEditor);
            handleDocumentChange();
        }
+


Get rid of this blank line.

humphd · 2017-04-03T14:30:45Z

src/extensions/default/Brackets-InteractiveLinter/main.js

     */
    function appReady() {
+        loadPreferences(); 
        // Removes the default Brackets JSLint linter


Because we are going to disable everything by default, and because it will add a lot of network size when we first load the app, I think we should do lazy-loading of all the linting plugins. To do this will requires a few things:

Move all of the code from 172 to 205 into a function named init. This init function will take care of loading all the plugins and registering them, as well as calling setDocument to enable any in the current editor.

After you call loadPreferences() on 171, do an if check to see if the enabled pref is true, and if it is, call init() immediately (i.e., if the user has the pref enabled=true, then we'll load all the plugins right now on startup). If it's false, add a one time event listener (e.g., the one() method) for the enabled pref changing to true, and call init() when that happens. This way we can wait until they opt-in to having these linters turned on before we do the step of loading everything over the network.

Does this make sense? When you're done doing all this, you should be able to test both ways of loading things by setting your default value for enabled above to true or false and then watching your dev tools Network tab to see what does and doesn't load in each case.

humphd · 2017-04-03T14:31:11Z

src/extensions/default/Brackets-InteractiveLinter/main.js

     */
    function setDocument(evt, currentEditor, previousEditor) {
+        if(!preferences.get("enabled")){
+            return ; 


Why the extra space between the n and the ;? Remove it.

humphd · 2017-04-10T16:31:28Z

@sgupta7857 it's been a week, what's up with this?

sgupta7857 · 2017-04-11T17:20:28Z

@humphd I am having trouble loading the plugin. Can you tell why is it happening?

humphd · 2017-04-13T01:50:06Z

src/extensions/default/Brackets-InteractiveLinter/main.js

+            init();
+        } else{
+            preferences.on("change", function handleChange(){
+                if(preferences.get("enabled"){


Missing a ) at the end.

humphd · 2017-04-13T02:02:40Z

src/extensions/default/Brackets-InteractiveLinter/main.js

+
+        function loadPreferences(){
+            preferences.definePreference("enabled", "boolean", defaultPreferences.enabled);
+            preferences.definePreference("javascript", "array", defaultPreferences.javascript); 


I did some debugging, and it's crashing when it tries to redefine this preference when the eslint provider is loaded. Remove this line (31) and line 32 below. In fact, just change lines 24-33 to this:

function addEnabledPref() { // We disable this by default preferences.definePreference("enabled", "boolean", false); }

humphd

I did some debugging and was able to get things working by fixing some issues you have in your code.

Also, you must fix your indenting. It's impossible to read the code when it's all misaligned. Take some time first thing and fix all your indents to 4-spaces.

humphd · 2017-04-21T17:13:09Z

src/extensions/default/Brackets-InteractiveLinter/main.js

        linterManager      = require("linterManager"),
        pluginManager      = require("pluginManager");
+
+	function addEnabledPref() {


This line is indented too far.

humphd · 2017-04-21T17:14:01Z

src/extensions/default/Brackets-InteractiveLinter/main.js

-        CodeInspection.register("javascript", {
+
+	function init(){
+		CodeInspection.register("javascript", {


This is indented too far.

humphd · 2017-04-21T17:14:44Z

src/extensions/default/Brackets-InteractiveLinter/main.js

+
+
+    function appReady() {
+        addEnabledPref()    


At this point, because it's a single line function, I think you should just inline this and put preferences.definePreference("enabled", "boolean", true); here vs. calling the function.

humphd · 2017-04-21T17:14:54Z

src/extensions/default/Brackets-InteractiveLinter/main.js

+        addEnabledPref()    
+        if (preferences.get("enabled")) {
+            init();
+        } 


humphd

I want you to fix the indenting in this file, and then figure out, once and for all, why you're getting it wrong all the time. Is it because you don't know how to do it correctly? If so, it's important that you learn to do it. Is it that you are going fast and didn't notice? If so, you need to slow down. Is it that you don't care? If so, I'd encourage you to rethink about the importance of code readability, since you're making mistakes due to your misalignment, and not being able to read the code for what it is.

If you need help understanding any of this, let me know. But let's not continue to have you get it wrong.

humphd · 2017-04-21T18:39:21Z

src/extensions/default/Brackets-InteractiveLinter/main.js


 	function init(){
-		CodeInspection.register("javascript", {
+	    CodeInspection.register("javascript", {


This entire function is indented incorrectly. I want you to actually pause for a minute, and look at what's here. There should be 4-spaces for each indent level. Look at what you have:

function init(){ CodeInspection.register("javascript", { name: "interactive-linter-remove-jslint", scanFile: $.noop }); // Load up plugins and wait til they are done loading before we // register any handlers into Brackets pluginManager().done(function(plugins) { for (var iPlugin in plugins) { linterManager.registerLinter(plugins[iPlugin]); } EditorManager.on("activeEditorChange.interactive-linter", setDocument); setDocument(null, EditorManager.getActiveEditor()); // If the linters change, then make sure to rebind the document with the new linter var lastLinters; preferences.on("change", function() { var editor = EditorManager.getActiveEditor(); if (!editor) { return; } var language = editor.document.getLanguage().getId(); var linters = preferences.get(language); if (linters && !_.isEqual(linters, lastLinters)) { lastLinters = linters.slice(0); handleDocumentChange(); } }); }); }

Just casually looking at that, you can see that it makes no sense. It looks like you're closing the init() function on the 4th line. The actual close is way down at the bottom, but it's impossible to see, because you haven't indented the pluginManager's done callback properly.

We've discussed this a bunch in my office, and I don't understand why we're still discussing it. Please fix all indentation issues in this PR, push it, and then come to Github and look at it to make sure it's right and that your editor isn't lying to you.

Here's what it should look like:

function init() { CodeInspection.register("javascript", { name: "interactive-linter-remove-jslint", scanFile: $.noop }); // Load up plugins and wait til they are done loading before we // register any handlers into Brackets pluginManager().done(function(plugins) { for (var iPlugin in plugins) { linterManager.registerLinter(plugins[iPlugin]); } EditorManager.on("activeEditorChange.interactive-linter", setDocument); setDocument(null, EditorManager.getActiveEditor()); // If the linters change, then make sure to rebind the document with the new linter var lastLinters; preferences.on("change", function() { var editor = EditorManager.getActiveEditor(); if (!editor) { return; } var language = editor.document.getLanguage().getId(); var linters = preferences.get(language); if (linters && !_.isEqual(linters, lastLinters)) { lastLinters = linters.slice(0); handleDocumentChange(); } }); }); }

humphd · 2017-04-21T18:41:04Z

src/extensions/default/Brackets-InteractiveLinter/main.js

-    }
-
-    require("lintIndicator");
+	require("lintIndicator");


Look at this. Surely you can see that it's not aligned with what's before/after it? Why is your indentation so wrong everywhere? Is your editor setup incorrectly?

humphd · 2017-04-21T18:41:38Z

src/extensions/default/Brackets-InteractiveLinter/main.js

                    init();
                }
-            });
+          });


This should be aligned with the preferences keyword on 197 above.

humphd

Still not right. I've tried to show you the problems visually. I get the feeling that you actually don't understand how to do this properly, which is an issue you will want to tackle, in general.

I'd recommend you install something like prettier in your editor to help you auto fix your code as you work: https://github.com/prettier/prettier.

I'd also suggest you put some serious time into figuring out when and how to indent manually. You need to know how to do this as a professional developer.

humphd · 2017-04-21T19:16:24Z

src/extensions/default/Brackets-InteractiveLinter/main.js

            handleDocumentChange();
        }
-    }
+	}


This is wrong. Take a look at this:

See how the increased indent you've added means that the function no longer lines up with its closing }?

humphd · 2017-04-21T19:18:51Z

src/extensions/default/Brackets-InteractiveLinter/main.js

-                if (!editor) {
-                    return;
-                }
+  function init() {


It looks like you've copy/pasted what I did, but neglected to indent it so that it lines up with the surrounding code. Look at this:

See how the function appReady no longer lines up with this? Everything in this init function needs to move over 1 tab (4-spaces) to line up.

humphd · 2017-04-21T19:20:31Z

src/extensions/default/Brackets-InteractiveLinter/main.js

            });
-
-        });
+          }


This is wrong. Look at this:

See how your if and else on lines 193 and 195 don't line up with this closing }? They all have to match.

sgupta7857 · 2017-04-21T22:48:59Z

for some reason, the code whatever indendation I fix on sublime doesn't match on github. I am using a different editor now Atom. Will fix the indentation and upload it
can you please review my code for release 0.4
error: My thimble isn't loading
sgupta7857#2

sgupta7857 · 2017-04-23T21:20:48Z

I used the preetier extension. hopefully it solves the indentation problem

humphd · 2017-04-23T21:34:11Z

Unfortunately this won't work. You've applied prettier's default styling (2-space indent) to the entire file, which you aren't changing here. The effect is that it's now impossible to see your actual changes, because it looks like every line has been changed.

If you want to undo this commit, you can do this:

git checkout -B disable-lint 67764868be51d2eb6dfb47ca4d749a4662b9168c
git push origin disable-lint -f

Then you can fix your indenting as I asked, and push again.

Fix mozilla/thimble.mozilla.org#2506 * Added all components for Border Radius Inline editor * remove text selection after closing inline border radius editor * made allCornerSlider and individual slider work seperately without effecting each other * added the slider value indicator * modified BorderRadiusEditor to allow keeping all units(%,em,px) * modified BorderRadiusEditor to allow keeping all units(%,em,px)--#2 * added radio buttons to toggle slider unit * making individual corner values sync with allcorner mode value * Border UI updates * fixed when close inlineEditor the cursor do not go to top * Light theme + CSS cleanup * code cleaned up * fixed indentation and replaceAll() local function * fixed some indentations * removed _values local property from borderRadiusEditor.js * Fixed few indentations and comments * added comment for borderRadiusUtil * added more comments in borderRadiusUtils.js * Fixed few comments * Review fixes * Fixed some codes according to comments from gide * fixed indentation * fixed indentation 2x

humphd and others added 5 commits March 4, 2017 17:09

Rework https://github.com/MiguelCastillo/Brackets-InteractiveLinter t…

2151d1a

…o load in browser

Deal with JS not being registered on startup

de60204

Fix eslint issues

6b0ce65

Rework variable decl for registered languages

319c52c

Changes

c339d4c

humphd reviewed Mar 27, 2017

View reviewed changes

sgupta7857 added 2 commits April 3, 2017 03:48

Disable lint in brackets by default

3b735cc

disable-lint in brackets by default

4bca551

humphd requested changes Apr 3, 2017

View reviewed changes

Plugin-loading issue

067f874

humphd reviewed Apr 13, 2017

View reviewed changes

humphd requested changes Apr 13, 2017

View reviewed changes

disable lint by default in brackets

a56e0dc

humphd requested changes Apr 21, 2017

View reviewed changes

changes

05cc7ec

humphd requested changes Apr 21, 2017

View reviewed changes

indent check

6776486

humphd requested changes Apr 21, 2017

View reviewed changes

sgupta7857 force-pushed the disable-lint branch from 4aafaad to 5eb1061 Compare April 23, 2017 21:18

sgupta7857 force-pushed the disable-lint branch from 5eb1061 to 6776486 Compare April 23, 2017 21:35

fix indentation

434a9be

humphd force-pushed the interactive-linting branch from 319c52c to fbc35d4 Compare July 20, 2017 21:08

		linterManager = require("linterManager"),
		pluginManager = require("pluginManager");

		pluginManager = require("pluginManager");


		var defaultPreferences = {

                           });
-                      });
+                        }

[WIP] issue #622 Disable lint by default #663 #2

Are you sure you want to change the base?

[WIP] issue #622 Disable lint by default #663 #2

Uh oh!

Conversation

sgupta7857 commented Mar 24, 2017

Uh oh!

sgupta7857 commented Mar 24, 2017

Uh oh!

humphd left a comment

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

humphd left a comment

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

humphd commented Apr 10, 2017

Uh oh!

sgupta7857 commented Apr 11, 2017

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

humphd left a comment

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

humphd left a comment

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

humphd left a comment

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

sgupta7857 commented Apr 21, 2017 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Uh oh!

sgupta7857 commented Apr 23, 2017

Uh oh!

humphd commented Apr 23, 2017

sgupta7857 commented Apr 21, 2017 •

edited

Loading