#4154 Reformat watchlogs.js indentation for consistency
Merged 7 months ago by tkopecek. Opened 8 months ago by ferdnyc.
ferdnyc/koji format-js  into  master

file added
+27
@@ -0,0 +1,27 @@ 

+ # Topmost editorconfig for project, don't ascend

+ # to parent directories when scanning configs

+ root = true

+ 

+ # All files: UTF-8 with Unix-style newlines,

+ # no trailing whitespace, and a final newline

+ [*]

+ charset = utf-8

+ end_of_line = lf

+ trim_trailing_whitespace = true

+ insert_final_newline = true

+ 

+ # JavaScript, CSS, Python, shell: 4-space indents

+ [{**/*.{js,css,py,sh},runtests,cli/koji,vm/kojivmd}]

+ indent_style = space

+ indent_size = 4

+ 

+ # Makefile: tab indents

+ [**/Makefile]

+ indent_style = tab

+ tab_width = 8

+ 

+ # CHTML: 2-space indents

+ [**/*.chtml]

+ indent_style = space

+ indent_size = 2

+ 

file modified
+1 -1
@@ -76,7 +76,7 @@ 

          [ -f /var/lock/subsys/kojira ] && restart || :

          ;;

    *)

- 	echo $"Usage: $0 {start|stop|status|restart|condrestart|try-restart|reload|force-reload}"

+         echo $"Usage: $0 {start|stop|status|restart|condrestart|try-restart|reload|force-reload}"

          exit 1

  esac

  

file modified
+1 -1
@@ -86,7 +86,7 @@ 

          graceful

          ;;

    *)

- 	echo $"Usage: $0 {start|stop|status|restart|condrestart|try-restart|reload|force-reload|graceful}"

+         echo $"Usage: $0 {start|stop|status|restart|condrestart|try-restart|reload|force-reload|graceful}"

          exit 1

  esac

  

@@ -17,13 +17,13 @@ 

      <tr>

        <th>Tags using this external repo</th>

        <td>

- 	#if $len($repoTags)

- 	#for $tag in $repoTags

- 	<a href="taginfo?tagID=$tag.tag_id">$util.escapeHTML($tag.tag_name)</a><br/>

- 	#end for

- 	#else

- 	No tags

- 	#end if

+         #if $len($repoTags)

+         #for $tag in $repoTags

+         <a href="taginfo?tagID=$tag.tag_id">$util.escapeHTML($tag.tag_name)</a><br/>

+         #end for

+         #else

+         No tags

+         #end if

        </td>

      </tr>

    </table>

file modified
+9 -9
@@ -10,24 +10,24 @@ 

      #if $tag

      <tr>

        <td colspan="5">

- 	<table class="nested">

- 	  <tr><td>

+         <table class="nested">

+           <tr><td>

            <strong>Inherited</strong>:

- 	  </td><td>

+           </td><td>

            <select name="inherited" class="filterlist" onchange="javascript: window.location = 'packages?inherited=' + this.value + '$util.passthrough_except($self, 'inherited')';">

              <option value="1" #if $inherited then 'selected="selected"' else ''#>yes</option>

              <option value="0" #if not $inherited then 'selected="selected"' else ''#>no</option>

            </select>

- 	  </td></tr>

- 	  <tr><td>

+           </td></tr>

+           <tr><td>

            <strong>With blocked</strong>:

- 	  </td><td>

+           </td><td>

            <select name="blocked" class="filterlist" onchange="javascript: window.location = 'packages?blocked=' + this.value + '$util.passthrough_except($self, 'blocked')';">

              <option value="1" #if $blocked then 'selected="selected"' else ''#>yes</option>

              <option value="0" #if not $blocked then 'selected="selected"' else ''#>no</option>

            </select>

- 	  </td></tr>

- 	</table>

+           </td></tr>

+         </table>

      </tr>

      #end if

      <tr>
@@ -38,7 +38,7 @@ 

          #else

          <a href="packages?prefix=$char$util.passthrough($self, 'userID', 'tagID', 'order', 'inherited', 'blocked')">$char</a>

          #end if

-         | 

+         |

          #end for

          #if $prefix

          <a href="packages?${util.passthrough($self, 'userID', 'tagID', 'order', 'inherited', 'blocked')[1:]}">all</a>

file modified
+7 -7
@@ -102,13 +102,13 @@ 

      <tr>

        <th>External&nbsp;repos</th>

        <td>

- 	#for $external_repo in $external_repos

- 	<a href="externalrepoinfo?extrepoID=$external_repo.external_repo_id">$util.escapeHTML($external_repo.external_repo_name)</a> [$external_repo.merge_mode]

- 	#if $external_repo.tag_id != $tag.id

- 	<span class="smaller">(inherited from <a href="taginfo?tagID=$external_repo.tag_id">$util.escapeHTML($external_repo.tag_name)</a>)</span>

- 	#end if

- 	<br/>

- 	#end for

+         #for $external_repo in $external_repos

+         <a href="externalrepoinfo?extrepoID=$external_repo.external_repo_id">$util.escapeHTML($external_repo.external_repo_name)</a> [$external_repo.merge_mode]

+         #if $external_repo.tag_id != $tag.id

+         <span class="smaller">(inherited from <a href="taginfo?tagID=$external_repo.tag_id">$util.escapeHTML($external_repo.tag_name)</a>)</span>

+         #end if

+         <br/>

+         #end for

        </td>

      </tr>

      #end if

file modified
+83 -83
@@ -39,62 +39,62 @@ 

  function handleStatus(event) {

      req = event.target;

      if (req.readyState != 4) {

- 	return;

+         return;

      }

      if (req.status == 200) {

- 	if (req.responseText.length > 0) {

- 	    var lines = req.responseText.split("\n");

- 	    var line = lines[0];

- 	    var data = line.split(":");

- 	    // var taskID = parseInt(data[0]);

- 	    var state = data[1];

- 	    var logs = {};

- 	    for (var i = 1; i < lines.length; i++) {

- 		data = lines[i].split(":");

- 		var filename = data[0] + ":" + data[1];

- 		var filesize = parseInt(data[2]);

- 		if (filename.indexOf(".log") != -1) {

- 		    logs[filename] = filesize;

- 		}

- 	    }

- 	} else {

- 	    // task may not have started yet

- 	    var state = "UNKNOWN";

- 	    var logs = {};

- 	}

- 	currentInfo = {state: state, logs: logs};

- 	if (!(state == "FREE" || state == "OPEN" ||

- 	      state == "ASSIGNED" || state == "UNKNOWN")) {

- 	    // remove tasks from the task list that are no longer running

- 	    for (var i = 0; i < tasks.length; i++) {

- 		if (tasks[i] == currentTaskID) {

- 		    tasks.splice(i, 1);

- 		    break;

- 		}

- 	    }

- 	}

+         if (req.responseText.length > 0) {

+             var lines = req.responseText.split("\n");

+             var line = lines[0];

+             var data = line.split(":");

+             // var taskID = parseInt(data[0]);

+             var state = data[1];

+             var logs = {};

+             for (var i = 1; i < lines.length; i++) {

+                 data = lines[i].split(":");

+                 var filename = data[0] + ":" + data[1];

+                 var filesize = parseInt(data[2]);

+                 if (filename.indexOf(".log") != -1) {

+                     logs[filename] = filesize;

+                 }

+             }

+         } else {

+             // task may not have started yet

+             var state = "UNKNOWN";

+             var logs = {};

+         }

+         currentInfo = {state: state, logs: logs};

+         if (!(state == "FREE" || state == "OPEN" ||

+               state == "ASSIGNED" || state == "UNKNOWN")) {

+             // remove tasks from the task list that are no longer running

+             for (var i = 0; i < tasks.length; i++) {

+                 if (tasks[i] == currentTaskID) {

+                     tasks.splice(i, 1);

+                     break;

+                 }

+             }

+         }

      } else {

- 	currentInfo = {state: "UNKNOWN", logs: {}};

- 	popupError("Error checking status of task " + currentTaskID + ": " + req.statusText);

+         currentInfo = {state: "UNKNOWN", logs: {}};

+         popupError("Error checking status of task " + currentTaskID + ": " + req.statusText);

      }

      currentLogs = [];

      for (var logname in currentInfo.logs) {

- 	currentLogs.push(logname);

+         currentLogs.push(logname);

      }

      processLog();

  }

  

  function getStatus() {

      if (tasksToProcess.length == 0) {

- 	if (errorCount > MAX_ERRORS) {

- 	    return;

- 	} else {

- 	    if (headerElement != null) {

- 		headerElement.appendChild(document.createTextNode("."));

- 	    }

- 	    setTimeout(checkTasks, 5000);

- 	    return;

- 	}

+         if (errorCount > MAX_ERRORS) {

+             return;

+         } else {

+             if (headerElement != null) {

+                 headerElement.appendChild(document.createTextNode("."));

+             }

+             setTimeout(checkTasks, 5000);

+             return;

+         }

      }

  

      currentTaskID = tasksToProcess.shift();
@@ -106,27 +106,27 @@ 

  

  function checkTasks() {

      if (tasks.length == 0) {

- 	docHeight = document.body.clientHeight;

+         docHeight = document.body.clientHeight;

          logElement.appendChild(document.createTextNode("\n==> Task has completed <==\n"));

          maybeScroll(docHeight);

      } else {

- 	tasksToProcess = [];

- 	for (var i = 0; i < tasks.length; i++) {

- 	    tasksToProcess.push(tasks[i]);

- 	}

- 	getStatus();

+         tasksToProcess = [];

+         for (var i = 0; i < tasks.length; i++) {

+             tasksToProcess.push(tasks[i]);

+         }

+         getStatus();

      }

  }

  

  function processLog() {

      if (currentLogs.length == 0) {

- 	getStatus();

- 	return;

+         getStatus();

+         return;

      }

      currentLog = currentLogs.shift();

      var taskOffsets = offsets[currentTaskID];

      if (!(currentLog in taskOffsets)) {

- 	taskOffsets[currentLog] = 0;

+         taskOffsets[currentLog] = 0;

      }

      outputLog();

  }
@@ -135,47 +135,47 @@ 

      var currentOffset = offsets[currentTaskID][currentLog];

      var currentSize = currentInfo.logs[currentLog];

      if (currentSize > currentOffset) {

- 	var chunkSize = CHUNK_SIZE;

- 	if ((currentSize - currentOffset) < chunkSize) {

- 	    chunkSize = currentSize - currentOffset;

- 	}

- 	var req = new XMLHttpRequest();

- 	var data = currentLog.split(':');

- 	var volume = data[0];

- 	var filename = data[1];

- 	req.open("GET", baseURL + "/getfile?taskID=" + currentTaskID + "&name=" + filename +

+         var chunkSize = CHUNK_SIZE;

+         if ((currentSize - currentOffset) < chunkSize) {

+             chunkSize = currentSize - currentOffset;

+         }

+         var req = new XMLHttpRequest();

+         var data = currentLog.split(':');

+         var volume = data[0];

+         var filename = data[1];

+         req.open("GET", baseURL + "/getfile?taskID=" + currentTaskID + "&name=" + filename +

                   "&volume=" + volume + "&offset=" + currentOffset + "&size=" + chunkSize, true);

- 	req.onreadystatechange = handleLog;

- 	req.send(null);

- 	if (headerElement != null) {

- 	    logElement.removeChild(headerElement);

- 	    headerElement = null;

- 	}

+         req.onreadystatechange = handleLog;

+         req.send(null);

+         if (headerElement != null) {

+             logElement.removeChild(headerElement);

+             headerElement = null;

+         }

      } else {

- 	processLog();

+         processLog();

      }

  }

  

  function handleLog(event) {

      req = event.target;

      if (req.readyState != 4) {

- 	return;

+         return;

      }

      if (req.status == 200) {

- 	content = req.responseText;

- 	offsets[currentTaskID][currentLog] += content.length;

- 	if (content.length > 0) {

- 	    docHeight = document.body.clientHeight;

- 	    currlog = currentTaskID + ":" + currentLog;

- 	    if (currlog != lastlog) {

- 		logElement.appendChild(document.createTextNode("\n==> " + currlog + " <==\n"));

- 		lastlog = currlog;

- 	    }

- 	    logElement.appendChild(document.createTextNode(content));

- 	    maybeScroll(docHeight);

- 	}

+         content = req.responseText;

+         offsets[currentTaskID][currentLog] += content.length;

+         if (content.length > 0) {

+             docHeight = document.body.clientHeight;

+             currlog = currentTaskID + ":" + currentLog;

+             if (currlog != lastlog) {

+                 logElement.appendChild(document.createTextNode("\n==> " + currlog + " <==\n"));

+                 lastlog = currlog;

+             }

+             logElement.appendChild(document.createTextNode(content));

+             maybeScroll(docHeight);

+         }

      } else {

- 	popupError("Error retrieving " + currentLog + " for task " + currentTaskID + ": " + req.statusText);

+         popupError("Error retrieving " + currentLog + " for task " + currentTaskID + ": " + req.statusText);

      }

      outputLog();

  }

watchlogs.js was a mess of mixed indentation, with some sections using 8-column tabs with 4-column indents, and others using spaces for indentation throughout. As a result, the formatting was erratic and difficult to read, particularly when viewed with anything other than an 8-column tab width. (Like Pagure's own diff views, where initial tabs are 6 spaces because of the -/+ markers.)

Reformatted with 4-space indents throughout.

This seems fine. Confirmed that the changes are only whitespace.

I think some folks prefer tabs in javascript for character count reasons, but in a world where pages routinely load megs and megs of js files it seems like a useless optimization. Better to use spaces as you suggest.

We could minimize it in Makefile, but as you say - it is almost irrelevant there days.

I'm fine with tab indents, as long as they're used consistently — meaning, 1 tab per indent level. None of this "4-column indents, but every 2 levels is a tab" nonsense, that always breaks. If people want to use tabs and also want indents to be 4 columns, they can set their tab width to 4.

And, it's true that the spaces increase the file size. It's not a very big file, but the existing file is 5316 bytes. Converting to space indents (this PR) blows that up to 5988 bytes. Still not big, but quite a bit bigger.

An all-tabs-indented version (on my format-js-2 branch) that uses spaces only for alignment on wrapped calls (one 2-line if and the remaining arguments to one long req.open()) is only 5088 bytes. I can open a PR with that change, instead, if you prefer?

I'd stick with spaces. The tabs were almost certainly there by accident

1 new commit added

  • Add preliminary .editorconfig rules
8 months ago

I added a second commit creating a preliminary .editorconfig in the repo root, formalizing some of these rules.

So far it only has:

  1. All files are UTF-8, LF endings, no trailing whitespace, final newline
  2. *.js and *.py use 4-space indents
  3. Makefile uses 8-column tab indents

I can add more rules for other file types, if anyone has suggestions?

Hmm. Are the .sql files in schemas/ generated, or hand-coded? Because, looking at schema.sql, seems it's also besieged by tab- and space-indented anarchy.

(At least the indentation's consistent: 8 columns. It just can't decide how it wants to get from column 1 to column 8.)

Just added *.css and *.sh to the 4-space indent section (matches format of existing files), and listed some extensionless Python files (runtests, cli/koji, vm/kojivmd) explicitly so the rules will apply to them as well.

1 new commit added

  • More .editorconfig rules
8 months ago

2 new commits added

  • Add preliminary .editorconfig rules
  • Reformat to 4-space indents throughout
8 months ago

Still a bit more:

$ rg -l \\t |grep -v Makefile|grep -v schema|grep -v watchlogs
LGPL
devtools/kojipolicy.vim
util/kojira.init
www/kojiweb/externalrepoinfo.chtml
www/kojiweb/taginfo.chtml
www/kojiweb/packages.chtml
vm/kojivmd.init
tests/test_lib/data/specs/test-files._spec
tests/test_lib/data/specs/test-src._spec
tests/test_lib/data/specs/test-deps._spec

Thanks, @tkopecek

LGPL

Definitely can't touch that. Learned my lesson there.

devtools/kojipolicy.vim
tests/test_lib/data/specs/test-files._spec
tests/test_lib/data/specs/test-src._spec
tests/test_lib/data/specs/test-deps._spec

These tabs all look intentional. I'm reluctant to mess with them.

util/kojira.init
vm/kojivmd.init

Heh. There was one tab in each file. Fixing.

www/kojiweb/externalrepoinfo.chtml
www/kojiweb/taginfo.chtml
www/kojiweb/packages.chtml

Yeah, these are also a mess of mixed indentation. Will try to sort them out. And it looks like .chtml should use 2-space indents, will add to .editorconfig.

2 new commits added

  • De-tabify .chtml files
  • Init files: expand tab indents
8 months ago

Coming back to the schema files, only 7 of 35 contain any tabs, and mostly not too many — except for the main schema.sql which is mostly tabs.

$ grep -Ec '^ ' schema.sql
143
$ grep -c '     ' *.sql|grep -v ':0$'
schema.sql:435
schema-upgrade-1.17-1.18.sql:3
schema-upgrade-1.18-1.19.sql:13
schema-upgrade-1.21-1.22.sql:1
schema-upgrade-1.2-1.3.sql:24
schema-upgrade-1.9-1.10.sql:13

I was also wrong about the consistency in how they're used. There are (based on the surrounding space-indented code) sections formatted for 4-column tabs, sections formatted for 8-column tabs, lines with 8-space indents, lines with 4-space indents, and this one procedure (schema.sql lines 813-824) that uses a 3-space initial indent, then 4-space indents after that:

-- index for default search method for rpms, PG11+ can benefit from new include method
DO $$
   DECLARE version integer;
   BEGIN
       SELECT current_setting('server_version_num')::integer INTO version;
       IF version >= 110000 THEN
           EXECUTE 'CREATE INDEX rpminfo_filename ON rpminfo((name || ''-'' || version || ''-'' || release || ''.'' || arch || ''.rpm'')) INCLUDE (id);';
       ELSE
           EXECUTE 'CREATE INDEX rpminfo_filename ON rpminfo((name || ''-'' || version || ''-'' || release || ''.'' || arch || ''.rpm''));';
       END IF;
   END
$$;

I could standardize all of that (though the question remains: around what rules?), but it's going to mean major reformatting to the main schema.sql in particular.

And even the files without tabs aren't entirely consistent. schema-upgrade-1.34-1.35.sql, for example, uses 8-space indents... except for the last line, which has a 4-space indent.

Maybe it's best to just leave these alone?

devtools/kojipolicy.vim
tests/test_lib/data/specs/test-files._spec
tests/test_lib/data/specs/test-src._spec
tests/test_lib/data/specs/test-deps._spec

These tabs all look intentional. I'm reluctant to mess with them.

More to the point, they're all inner tabs, not indentation.

(re: schemas/*.sql)

Maybe it's best to just leave these alone?

If so, then this PR is complete. I've addressed every tab-containing file except the schemas/ dir and the "untouchables" (the license file and the ones listed above).

If I should reformat the schemas/ files, then someone please define the set of indentation rules they should follow, as the existing formatting is inconsistent enough that there's no one obvious choice.

@mikem @tkopecek Anything else you need from me on this?

issue #4187

I would leave it as it is now. Thanks! (scheduled for next minor release)

Metadata Update from @tkopecek:
- Pull-request tagged with: no_qe

7 months ago

Commit dc3eede fixes this pull-request

Pull-Request has been merged by tkopecek

7 months ago