Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

wrap_line_length wrapping sooner than it should #1677

Open
kasvtv opened this issue May 26, 2019 · 2 comments
Open

wrap_line_length wrapping sooner than it should #1677

kasvtv opened this issue May 26, 2019 · 2 comments

Comments

@kasvtv
Copy link

kasvtv commented May 26, 2019

Description

I can see this has been a problem before (#1401), but this issue still exists.
See line 29. It is at 103 out of 120 possible columns, yet it still gets wrapped.

Input

The code looked like this before beautification:

var parseRequestHeaders = require('micro-xhr/lib/parseRequestHeaders');

function isJson(headers) {
	return (headers['content-type'] || '').toLowerCase().indexOf('application/json') !== -1;
}

module.exports = function xhrWrapper(args) {
	var xhr;
	var promise = new Promise(function(resolve) {
		xhr = args.xhrInstance || new XMLHttpRequest();

		xhr.open(args.method || 'get', args.url);

		var lowercaseHeaders = parseRequestHeaders(args.headers);

		for (var name in lowercaseHeaders) {
			xhr.setRequestHeader(name.toLowerCase(), lowercaseHeaders[name]);
		}

		xhr.onreadystatechange = function() {
			if (xhr.readyState === this.DONE) {
				var responseHeaders = {};

				xhr.getAllResponseHeaders()
					.split(/(\r)?\n/)
					.forEach(function(x) {
						if (!x) return;
						var separator = x.indexOf(': ');
						responseHeaders[x.slice(0, separator).toLowerCase()] = x.slice(separator + 2);
					});

				var responseData = isJson(responseHeaders)
					? JSON.parse(xhr.responseText)
					: xhr.responseText;

				resolve({
					status: xhr.status,
					data: responseData,
					headers: responseHeaders,
				});
			}
		};

		xhr.send(
			isJson(lowercaseHeaders) && args.data
			? JSON.stringify(args.data)
			: args.data
		);
	});

	promise.xhr = xhr;

	return promise;
};

Expected Output

The code should have looked like this after beautification:

var parseRequestHeaders = require('micro-xhr/lib/parseRequestHeaders');

function isJson(headers) {
	return (headers['content-type'] || '').toLowerCase().indexOf('application/json') !== -1;
}

module.exports = function xhrWrapper(args) {
	var xhr;
	var promise = new Promise(function(resolve) {
		xhr = args.xhrInstance || new XMLHttpRequest();

		xhr.open(args.method || 'get', args.url);

		var lowercaseHeaders = parseRequestHeaders(args.headers);

		for (var name in lowercaseHeaders) {
			xhr.setRequestHeader(name.toLowerCase(), lowercaseHeaders[name]);
		}

		xhr.onreadystatechange = function() {
			if (xhr.readyState === this.DONE) {
				var responseHeaders = {};

				xhr.getAllResponseHeaders()
					.split(/(\r)?\n/)
					.forEach(function(x) {
						if (!x) return;
						var separator = x.indexOf(': ');
						responseHeaders[x.slice(0, separator).toLowerCase()] = x.slice(separator + 2);
					});

				var responseData = isJson(responseHeaders)
					? JSON.parse(xhr.responseText)
					: xhr.responseText;

				resolve({
					status: xhr.status,
					data: responseData,
					headers: responseHeaders,
				});
			}
		};

		xhr.send(
			isJson(lowercaseHeaders) && args.data
			? JSON.stringify(args.data)
			: args.data
		);
	});

	promise.xhr = xhr;

	return promise;
};

Actual Output

The code actually looked like this after beautification:

var parseRequestHeaders = require('micro-xhr/lib/parseRequestHeaders');

function isJson(headers) {
	return (headers['content-type'] || '').toLowerCase().indexOf('application/json') !== -1;
}

module.exports = function xhrWrapper(args) {
	var xhr;
	var promise = new Promise(function(resolve) {
		xhr = args.xhrInstance || new XMLHttpRequest();

		xhr.open(args.method || 'get', args.url);

		var lowercaseHeaders = parseRequestHeaders(args.headers);

		for (var name in lowercaseHeaders) {
			xhr.setRequestHeader(name.toLowerCase(), lowercaseHeaders[name]);
		}

		xhr.onreadystatechange = function() {
			if (xhr.readyState === this.DONE) {
				var responseHeaders = {};

				xhr.getAllResponseHeaders()
					.split(/(\r)?\n/)
					.forEach(function(x) {
						if (!x) return;
						var separator = x.indexOf(': ');
						responseHeaders[x.slice(0, separator).toLowerCase()] = x.slice(separator +
							2);
					});

				var responseData = isJson(responseHeaders)
					? JSON.parse(xhr.responseText)
					: xhr.responseText;

				resolve({
					status: xhr.status,
					data: responseData,
					headers: responseHeaders,
				});
			}
		};

		xhr.send(
			isJson(lowercaseHeaders) && args.data
			? JSON.stringify(args.data)
			: args.data
		);
	});

	promise.xhr = xhr;

	return promise;
};

Settings

Example:

{
	"indent_with_tabs": true,
	"indent_size": 4,
	"operator_position": "after-newline",
	"max_preserve_newlines": 3,
	"preserve_newlines": true,
	"space_before_conditional": true,
	"space_in_empty_paren": false,
	"unescape_strings": true,
	"wrap_line_length": 120,
	"brace_style": "collapse,preserve-inline",
	"extra-liners": []
}
@bitwiseman
Copy link
Member

bitwiseman commented May 26, 2019

@kasvtv
Thanks for the excellent bug report.
Yeah, line wrapping is hard.

The beautifier only recently got a fix to guarantee always wrapping at or before the limit.

However, it also has as a function to remove extra indents. The wrapping occurs before the extra indent removal.

This code starts off with two indents but then then an extra indent is removed:

                                        //      v=indent-1   v=indent-2
					.forEach(function(x) {
                                             ...
					});
                                     //  ^=indent-1  removed as redundant

However, line wrapping needs to happen before extra indents are calculated, because extra indents are based on whether a block has multiple lines or not.

On the other hand, I think most redundant indents could be calculated during parsing. The one in this example definitely could.
It just needs someone with the time to do it.

@smujaddid
Copy link

Any fix for this?

This happens too with wrap_line_length set to 120 for this line:

Code

          this.$watch('posisiRunningText', (value) => (this.posisiRunningTextClass = 'running-text-' + value))

Expected

Stay the same

Actual

          this.$watch('posisiRunningText', (value) => (this.posisiRunningTextClass = 'running-text-' +
            value))

Settings

{
  "brace_style": "collapse,preserve-inline",
  "break_chained_methods": false,
  "comma_first": false,
  "e4x": false,
  "end_with_newline": false,
  "indent_char": " ",
  "indent_empty_lines": false,
  "indent_inner_html": false,
  "indent_scripts": "normal",
  "indent_size": "2",
  "jslint_happy": false,
  "keep_array_indentation": false,
  "max_preserve_newlines": "5",
  "preserve_newlines": true,
  "space_before_conditional": true,
  "unescape_strings": false,
  "wrap_attributes": "auto",
  "wrap_attributes_indent_size": 2,
  "wrap_line_length": "120"
}

Note: I'm using a VSCode plugin https://github.com/HiroyukiNIshimura/ejs-beautify for this

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Projects
None yet
Development

No branches or pull requests

3 participants