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

fix(cli): allow multiple occurrences of the same name in user properties #1157

Merged
merged 2 commits into from
Nov 30, 2022

Conversation

Red-Asuka
Copy link
Member

@Red-Asuka Red-Asuka commented Nov 30, 2022

PR Checklist

If you have any questions, you can refer to the Contributing Guide

What is the current behavior?

Please describe the current behavior and link to a relevant issue.

Issue Number

Example: #123

What is the new behavior?

image

Does this PR introduce a breaking change?

  • Yes
  • No

Specific Instructions

Are there any specific instructions or things that should be known prior to review?

Other information

@Red-Asuka Red-Asuka added fix Fix bug or issues CLI MQTTX CLI labels Nov 30, 2022
@Red-Asuka Red-Asuka self-assigned this Nov 30, 2022
Comment on lines +41 to +42
return { [key]: val }
} else {
Copy link
Member

@ysfscream ysfscream Nov 30, 2022

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

else is needless after the return statement.

if (!previous) return { [key]: value }
if (xxx) {
    xxxx
}

Copy link
Member Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I know that else is unnecessary, but I think that in some cases else can make the code clearer. This is the code without else, what do you think?

    if (!previous) return { [key]: val }
    if (previous[key]) {
      if (Array.isArray(previous[key])) {
        ;(previous[key] as string[]).push(val)
      } else {
        previous[key] = [previous[key] as string, val]
      }
      return previous
    } else {
      return { ...previous, [key]: val }
    }

Copy link
Member Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

The final code:

    if (!previous) return { [key]: val }
    if (previous[key]) {
      if (Array.isArray(previous[key])) {
        const currentVal = previous[key] as string[]
        currentVal.push(val)
        return { ...previous, [key]: currentVal }
      }
      previous[key] = [previous[key] as string, val]
      return previous
    }
    return { ...previous, [key]: val }

Copy link
Member Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Or

    if (!previous) return { [key]: val }
    if (!previous[key]) return { ...previous, [key]: val }
    if (Array.isArray(previous[key])) {
      const currentVal = previous[key] as string[]
      currentVal.push(val)
      return { ...previous, [key]: currentVal }
    }
    previous[key] = [previous[key] as string, val]
    return previous

Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

else needs continuity, and return means execution is over. It doesn't say here that else doesn't need to be used, I think this code is just fine

if (!previous) return { [key]: val }
if (previous[key]) {
  if (Array.isArray(previous[key])) {
	const currentVal = previous[key] as string[]
     currentVal.push(val)
  } else {
  	const currentVal = previous[key] as string
    previous[key] = [currentVal, val]
  }
  return previous
}
 return { ...previous, [key]: val }

Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Anyway, I will merge first. But the principle of my judgment is that if there is a return in the if, there will be no need for else; if not, then there is a need for else; this proposal should be in the rules of Eslint also had, personal preference, code readability is our goal.

Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

It's a nice job.

} else {
if (previous[key]) {
if (Array.isArray(previous[key])) {
;(previous[key] as string[]).push(val)
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Please declare an internal variable to represent previous[key], For example:

const objValues = previous[key] as string[]

Looks cumbersome, but more clear type

@ysfscream ysfscream merged commit 1bd3000 into emqx:main Nov 30, 2022
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
CLI MQTTX CLI fix Fix bug or issues
Projects
None yet
Development

Successfully merging this pull request may close these issues.

2 participants