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

Rename "showConnectionOptions" state setter to "setConnectOptionsShown" #850

Merged
merged 1 commit into from Mar 23, 2022

Conversation

misicnenad
Copy link
Collaborator

Rename to "set<>" format just to follow the React convention.

@ap-justin
Copy link
Collaborator

@misicnenad, I don't think this is a must follow convention, it's a must though that names suggest that one is state and the other setState. For this case, it's clear that connectOptionsShown is state and showConnectionOptions as something that changes that state

@misicnenad
Copy link
Collaborator Author

misicnenad commented Mar 22, 2022

@misicnenad, I don't think this is a must follow convention, it's a must though that names suggest that one is state and the other setState. For this case, it's clear that connectOptionsShown is state and showConnectionOptions as something that changes that state

@ap-justin for me it wasn't so obvious that showConnectionOptions was a state setter, looked more like a regular function like any other.
I mean, you are basically right, it is not hard-and-fast rule (at least for now, see end of this comment). But personally, seeing "set" as prefix to a variable "x" helps me to understand this variable is responsible for updating state of "x". Conventions are there to make understanding data types and the purpose of the variables as easy as possible, so why not follow them (unless there's a good reason)?

Additionally, it seems this convention will become more or less enforced in future iterations of eslint-plugin-react, so we might as well update this now.

@ap-justin
Copy link
Collaborator

@misicnenad, I don't think this is a must follow convention, it's a must though that names suggest that one is state and the other setState. For this case, it's clear that connectOptionsShown is state and showConnectionOptions as something that changes that state

@ap-justin for me it wasn't so obvious that showConnectionOptions was a state setter, looked more like a regular function like any other. I mean, you are basically right, it is not hard-and-fast rule (at least for now, see end of this comment). But personally, seeing "set" as prefix to a variable "x" helps me to understand this variable is responsible for updating state of "x". Conventions are there to make understanding data types and the purpose of the variables as easy as possible, so why not follow them (unless there's a good reason)?

Additionally, it seems this convention will become more or less enforced in future iterations of eslint-plugin-react, so we might as well update this now.

@misicnenad, I got this reconsidered and saw that your point is good actually

const [isDropdownOpen, setIsDropdownOpen] = useState(false)

function openDropdown(){
  setIsDropdownOpen(true) 
}

@SovereignAndrey SovereignAndrey merged commit 9021911 into master Mar 23, 2022
@SovereignAndrey SovereignAndrey deleted the rename-state-setter branch March 23, 2022 02:46
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

None yet

3 participants