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

cap line length for .kt files to 80 in src/docs #790

Merged
merged 7 commits into from Oct 2, 2022

Conversation

p10r
Copy link
Contributor

@p10r p10r commented Oct 2, 2022

Some of the docs currently look like this:

image

One has to use horizontal scrolling to see the whole code (Firefox, MBP 2019, no zoom)

Suggestion: Applying max_line_length = 80 for all Kotlin files in src/docs, so everything fits on the screen without scrolling.

Searched around for this and it seems like this was already a thing in the slack channel, but seems like the PR was never opened(?).

@p10r
Copy link
Contributor Author

p10r commented Oct 2, 2022

Anyway, wanted to check back before I go through every file and check what the IDEA formatter did.
What do you think about altering the .editorconfig for this?

@codecov-commenter
Copy link

codecov-commenter commented Oct 2, 2022

Codecov Report

Merging #790 (d614bb4) into master (6b2d17a) will not change coverage.
The diff coverage is n/a.

❗ Current head d614bb4 differs from pull request most recent head f4092a7. Consider uploading reports for the commit f4092a7 to get more accurate results

@@            Coverage Diff            @@
##             master     #790   +/-   ##
=========================================
  Coverage     84.65%   84.65%           
  Complexity     1548     1548           
=========================================
  Files           473      473           
  Lines         10768    10768           
  Branches       1400     1400           
=========================================
  Hits           9116     9116           
  Misses          974      974           
  Partials        678      678           

📣 We’re building smart automated test selection to slash your CI/CD build times. Learn more

@daviddenton
Copy link
Member

Yeah, seems like a legit change. Happy to do it if we can limit the length just in the docs folder (I think we use 120 elsewhere)

@p10r
Copy link
Contributor Author

p10r commented Oct 2, 2022

Yeah, checked it and it's working as intended. It's also only targeting Kotlin files, nothing else.

Let me take care of it and have a look later on. 👍

.editorconfig Outdated
Comment on lines 23 to 24
[src/docs/*/**.kt]
max_line_length = 80
Copy link
Contributor Author

Choose a reason for hiding this comment

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

Most important part

fun readFile(secretKey: String, bucketKey: String): ByteBuffer {
return s3(Request(GET, "https://mybucket.s3.amazonaws.com/$bucketKey")).body.payload
}
fun readFile(secretKey: String, bucketKey: String): ByteBuffer =
Copy link
Contributor Author

@p10r p10r Oct 2, 2022

Choose a reason for hiding this comment

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

secretKey is not being used - not sure if intended. (Corresponding Docs)

@razvn
Copy link
Contributor

razvn commented Oct 2, 2022

Please not 80 we're no longer in old Unix system, we have 4k-6k screens I hate all the white space on the web because everyone thing we are still in 1024px...

Anyway 120 is what fits on a standard HD screen with left and right panels of IntelliJ so IMHO is a minimum and a good enough compromise.

@daviddenton
Copy link
Member

@razvn this is only for the website docs content, so am happy to go with whatever fits on most people's screens. Seems to be about 95 on my screen. Definitely NOT the IDE 😄

@p10r
Copy link
Contributor Author

p10r commented Oct 2, 2022

Hey @razvn, it's not about the general code style in the repo, it's just about the docs that are being shown when browsing the docs. There's a lot of horizontal scrolling:
image

@p10r
Copy link
Contributor Author

p10r commented Oct 2, 2022

Here's a better example with text above it and full screen:
image

@p10r
Copy link
Contributor Author

p10r commented Oct 2, 2022

We could also go for 95 @daviddenton - should also work fine. Wdyt?

@daviddenton
Copy link
Member

Yeah - 95 works better. Will be reasonable to code against whilst being readable.

Thanks for picking this up BTW @p10r - docs are important! 😄

Comment on lines +20 to +21
val portLens: Lens<Environment, Port> =
EnvironmentKey.int().map(::Port).defaulted("PORT", Port(9000))
Copy link
Contributor Author

Choose a reason for hiding this comment

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

We could also go for

val portLens: Lens<Environment, Port> = EnvironmentKey.int()
    .map(::Port)
    .defaulted("PORT", Port(9000))

But, if I remember correctly, the rest of the code base is not splitting lines after each method call, right?

@p10r
Copy link
Contributor Author

p10r commented Oct 2, 2022

Thanks for picking this up BTW @p10r - docs are important! 😄

This is also for my own reading experience, so happy to jump in there. 🌚

@p10r p10r marked this pull request as ready for review October 2, 2022 16:44
@p10r
Copy link
Contributor Author

p10r commented Oct 2, 2022

I also went through the markdown files. Looks good so far - I'll create a follow-up if I ever find another occurence.

@daviddenton daviddenton merged commit ba83d96 into http4k:master Oct 2, 2022
@daviddenton
Copy link
Member

thanks again :)

@p10r
Copy link
Contributor Author

p10r commented Oct 2, 2022

Thanks for building such a library ❤️

@p10r p10r deleted the cap-docs-line-length branch October 2, 2022 18:10
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

4 participants