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

Create FormName render props component #4038

Merged
merged 5 commits into from May 31, 2018

Conversation

jedwards1211
Copy link
Contributor

Based upon my suggestion in #4037

@danielrob
Copy link
Collaborator

This looks great. Could you just fix those code-climate eslint issues, and we'll await signoff/merge from Erik.

@danielrob danielrob requested a review from erikras May 23, 2018 04:10
@jedwards1211
Copy link
Contributor Author

jedwards1211 commented May 23, 2018

@danielrob okay, I fixed the unused variable errors. But the third one:

Function describeFormName has 43 lines of code (exceeds 25 allowed). Consider refactoring.

I really don't think this is a helpful rule, at least not in the test directory, and I decline to fix this issue in protest against it. I'd be willing to make a PR to turn off this rule in the test directory though.

@erikras erikras merged commit ca02205 into redux-form:master May 31, 2018
@jedwards1211
Copy link
Contributor Author

Sweet, thanks guys! At some point I'll get around to releasing my createStructuredFormSelector function in a package of its own.

@jedwards1211
Copy link
Contributor Author

jedwards1211 commented May 31, 2018

@erikras is the goal with the LOC rule to notify you guys when a human code review is needed, and if the length of the function is justified, eslint-disable it?
25 lines of code is a really low threshold, especially considering cases where we destructure several dozen props from an object, and put each prop on its own line for readability. Number of characters in the function after minification would be a better metric than number of lines, but really I'd rather see a more theoretical measure of code complexity if anything than just number of lines or characters in the function.

@jedwards1211
Copy link
Contributor Author

@erikras
Copy link
Member

erikras commented Jun 12, 2018

Released in v7.4.0.

@lock
Copy link

lock bot commented Jul 12, 2018

This thread has been automatically locked since there has not been any recent activity after it was closed. Please open a new issue for related bugs.

@lock lock bot locked as resolved and limited conversation to collaborators Jul 12, 2018
Sign up for free to subscribe to this conversation on GitHub. Already have an account? Sign in.
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

None yet

3 participants