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

Reduction in line character limit #48

Open
prdanelli opened this issue Mar 9, 2021 · 4 comments
Open

Reduction in line character limit #48

prdanelli opened this issue Mar 9, 2021 · 4 comments
Assignees

Comments

@prdanelli
Copy link

I have been working within the Hyku codespace for a little while and have been struggling with the higher character limit that you permit for each line of code - I believe this is currently set to 200 characters per line.

As someone who struggles periodically due to mild dyslexia, incredibly long lines of code are far harder to comprehend in their entirety than those which have been broken up for brevity.

I will provide an example below and please note I mean no disrespect to the owner of this code, it is only used because of its length of 184 characters - 16 character below your upper limit:

if Hyrax::PresenterFactory.build_for(ids: [collection.thumbnail_id], presenter_class: Hyrax::FileSetPresenter, presenter_args: [current_ability, request]).first&.solr_document&.public?

If this were broken up so that the options for the build_for method were on a seperate line, the entire condition can be much more easily comprehended:

options = { 
    ids: [collection.thumbnail_id], 
    presenter_class: Hyrax::FileSetPresenter, 
    presenter_args: [current_ability, request] 
} 
if Hyrax::PresenterFactory.build_for(options).first&.solr_document&.public?

Thank you.

@no-reply
Copy link

no-reply commented Mar 9, 2021

hi! thank you for this thoughtful issue!

i'm sorry that the long line limits are an accessibility issue for you, and i certainly agree that 200 is quite a bit too long for readability.

i'd suggest moving back toward the 80 character limit in The Ruby Style Guide.

one complication is that Bixby's main purpose is to insulate adopters from the frequent style changes once(?) included in rubocop version bumps. making the change here is something i'm not entirely sure we can count on having an impact on the code you're reading in the foreseeable future. in addition to making the change here, it would be worth engaging with the maintainers of the individual projects.

i'd be happy to spearhead this for hyrax---i might be a bit grateful to have a straightforward request to tidy up the long lines over there. :)

@prdanelli
Copy link
Author

Hello @no-reply,

Thank you for responding to me. I realise that this is a rather subjective issue, much like tabs vs. spaces. So might I suggest that, inline with the Ruby Style guide you mentioned above, that 120 characters is a compromise between the very long 200 characters lines and the a 1990s terminal width of 80 characters, whilst airing on the side of readability and increased comprehension.

For code maintained exclusively or primarily by a team that can reach agreement on this issue, it is okay to increase the line length limit up to 100 characters, or all the way up to 120 characters. Please, restrain the urge to go beyond 120 characters.

Thank you again for getting back to me and taking this issue so seriously.

@no-reply
Copy link

no-reply commented Mar 9, 2021

@prdanelli that seems about right. i think my personal practice for some years now has been a soft limit at 80, and a very hard limit at 120.

we should probably give it a bit for others to chime in, but in the meanwhile let's start thinking about how we can facilitate a gradual cleanup over at hyrax and hyku, at least.

@cjcolvar
Copy link
Member

cjcolvar commented Mar 9, 2021

This sounds like a potential developers congress issue both discussion and cleanup.

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

No branches or pull requests

3 participants