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

jsdoc/check-line-alignment adds extra spaces to objects #743

Closed
Allain55 opened this issue May 18, 2021 · 18 comments
Closed

jsdoc/check-line-alignment adds extra spaces to objects #743

Allain55 opened this issue May 18, 2021 · 18 comments

Comments

@Allain55
Copy link

Expected behavior

jsdoc/check-line-alignment should properly format objects with @param

Actual behavior

jsdoc/check-line-alignment adds many extra spaces for objects

ESLint Config

{
	'plugins': [
		'jsdoc'
	],
	'parser': 'babel-eslint',
	'parserOptions': {
		'ecmaVersion': 2021
	},
	'rules': {
		'jsdoc/check-line-alignment': ['error', 'always']
	}
};

ESLint sample

/**
 * @param {{
 * ids: number[]
 * }} params
 */
const myMethod = ({ids}) => {}

The result is:

/**
 * @param {{
 *                                                                       ids: number[]
 *                                                                       }}            params
 */
const myMethod = ({ids}) => {}

Environment

  • Node version: 16.0
  • ESLint version 7.26
  • eslint-plugin-jsdoc version: 34.7.0
@brettz9
Copy link
Collaborator

brettz9 commented May 18, 2021

@renatho , would you be able to take a look?

@renatho
Copy link
Contributor

renatho commented May 18, 2021

Hey there!

I tested this example, but it gave me the result as valid.
Are you able to reproduce it @brettz9?

@brettz9
Copy link
Collaborator

brettz9 commented May 18, 2021

Yeah, I'm getting these results (using tabs or spaces):

{
      /* eslint-disable no-tabs */
      code: `
				/**
				 * @param {{
				 * ids: number[]
				 * }} params
				 */
				const myMethod = ({ids}) => {}
			`,
      errors: [
        {
          line: 2,
          message: 'Expected JSDoc block lines to be aligned.',
          type: 'Block',
        },
      ],
      options: ['always'],
      output: `
				/**
				 * @param {{
				 *        ids: number[]
				 *        }}            params
				 */
				const myMethod = ({ids}) => {}
			`,
      /* eslint-enable no-tabs */
      parser: require.resolve('@babel/eslint-parser'),
      parserOptions: {
        ecmaVersion: 2_021,
      },
    },

(Btw, I got the same results with babel-eslint too, but it is probably a good idea to use @babel/eslint-parser instead as babel-eslint is deprecated in favor of it.)

@renatho
Copy link
Contributor

renatho commented May 18, 2021

I'm getting these results (using tabs or spaces)

You mean you got the issue? Or that it worked well for you? I tested the example you posted, and the tests passed.

@brettz9
Copy link
Collaborator

brettz9 commented May 18, 2021

yeah, sorry, I mean I got the same results as you--I don't see any issues.

brettz9 added a commit to brettz9/eslint-plugin-jsdoc that referenced this issue May 18, 2021
@brettz9
Copy link
Collaborator

brettz9 commented May 18, 2021

@Allain55 : Maybe you could set up a minimal test repo, as we're not able to replicate. I wouldn't think it would matter here, but are you on Windows?

@Allain55
Copy link
Author

Hi,
thanks for checking it.
I created a repo for this:
https://github.com/Allain55/eslint-plugin-jsdoc_743

I have Ubuntu 16.04.7 LTS

@brettz9
Copy link
Collaborator

brettz9 commented May 18, 2021

@renatho : You might want to look into it through the repo, as yeah, I can replicate with that repo.

@brettz9
Copy link
Collaborator

brettz9 commented May 18, 2021

FWIW, there is no need for babel-eslint to reproduce.

@brettz9
Copy link
Collaborator

brettz9 commented May 18, 2021

I'm guessing the issue--and the reason we don't see it in the tester, is that the solution the fixer provides might itself be detected as problematic, and some recursion takes place (ESLint will stop after I think 6 times).

@brettz9
Copy link
Collaborator

brettz9 commented May 18, 2021

Yeah, replacing with:

/**
 * @param {{
 *        ids: number[]
 *        }}            params
 */
const myMethod = ({ids}) => {}

...also causes recursive fixing. We need to make sure that the example above is not seen as misaligned or otherwise it will keep "fixing".

@renatho
Copy link
Contributor

renatho commented May 18, 2021

Thank you for creating a minimal setup for that @Allain55! And good catch @brettz9!

I was checking how we could solve that. I think it'll not be so simple. Also, the original align transform where the alignTransform was based on, doesn't handle that well.

I'm not sure when I'll have some more time to check this deeply, so if someone wanna pick up this. Feel free to. Otherwise, when I have some time, I'll be happy to investigate how to create a good solution for this. ;)

@brettz9
Copy link
Collaborator

brettz9 commented May 18, 2021

Is there maybe something we could auto-detect to at least prevent the fixer from auto-applying? E.g., if it is because of the multi-line type, just detecting that and stopping the fixer...

@renatho
Copy link
Contributor

renatho commented May 18, 2021

Is there maybe something we could auto-detect to at least prevent the fixer from auto-applying? E.g., if it is because of the multi-line type, just detecting that and stopping the fixer...

Yeah! Maybe it could be an easier workaround for now.

@renatho
Copy link
Contributor

renatho commented May 18, 2021

I created a workaround until it gets fixed, but I'm not so confident yet that we won't break any other case. I still wanna test it against the Gutenberg codebase that is big. If someone else is able to test against other codebases it would be interesting to check if we're not missing anything with this workaround.

It basically ignores lines that doesn't contain any tag, but contains type or name.

Notice that for now, it'll accept any type of alignment in these lines.

brettz9 added a commit that referenced this issue May 19, 2021
…xes part of #743

Workaround for recursive fixing of multiline type alignment

Co-authored-by: Renatho De Carli Rosa <renatho@automattic.com>
Co-authored-by: Brett Zamir <brettz9@yahoo.com>
@brettz9
Copy link
Collaborator

brettz9 commented Mar 11, 2022

@renatho : Do you have anything else you want to do for this per #743 (comment) ? Did you have an approach in mind to align multiline types? As we haven't gotten any other issues, I guess #744 at least hasn't apparently caused new issues.

@renatho
Copy link
Contributor

renatho commented Mar 11, 2022

Do you have anything else you want to do for this per #743 (comment) ? Did you have an approach in mind to align multiline types?

Hey @brettz9!

Nops! I don't have a good approach in mind, and I didn't have more plans for that. Maybe we could close this for now, and if people have related issues, we could just re-open it or create a new one. 😉

@brettz9
Copy link
Collaborator

brettz9 commented Mar 11, 2022

Thanks. An issue can be opened by anyone who does have a particular approach in mind, though no guarantees on whether we'll get to it or not.

@brettz9 brettz9 closed this as completed Mar 11, 2022
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