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

refactoring: format all code #56 #59

Merged
merged 1 commit into from
Jun 9, 2020
Merged

Conversation

vraybaud
Copy link
Contributor

@vraybaud vraybaud commented Jun 8, 2020

  • Import orders are now compliant with the Ktlint Intellij Settings. If you do ktlint, you'll find out "Imports must be ordered in lexicographic order without any empty lines in-between" errors, there are a known issue of ktlint <= 0.36.0 : Alphabetical import ordering enabled by default is a problem in Android Studio/IntelliJ pinterest/ktlint#527 which is fixed in v0.37.0, which requires an update from org.jlleitschuh.gradle.ktlint plugin.

  • Indentations are ajusted, removing useless indentations that were in many files.

  • Hard wrapping at 120 characters to ease the code review.

  • Add new line at end of file (see POSIX definition of a line)

Please if you see something strange, do comment it. (I could have missed something)

@vraybaud vraybaud requested review from bobeal and HoucemKacem June 8, 2020 12:16
@github-actions
Copy link
Contributor

github-actions bot commented Jun 8, 2020

CLA Assistant Lite All Contributors have signed the CLA.


StepVerifier.create(notificationService.callSubscriber(subscription, listOf(parsedEntity)))
.expectNextMatches {
it.first.id == subscription.id &&
it.second.subscriptionId == subscription.id &&
Copy link
Contributor

Choose a reason for hiding this comment

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

indentation seems to be not good here (and below).

Copy link
Contributor Author

Choose a reason for hiding this comment

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

This is to clearly identify the continuation of the expression, so you clearly see at first sight that this is all a single expression.

Maybe in this case it seems useless, but you could have cases like that :

val a = it.size == 1 &&
it[0].second.subscriptionId == subscription.id &&
it[0].second.data.size == 1 &&
it[0].third
a

There it becomes more relevant to have this indent to more easily detect the what is variable a and what is returned.

@HoucemKacem
Copy link
Contributor

Do we need to configure something in Intellij ?
From now on how can we check the code style ?

@vraybaud
Copy link
Contributor Author

vraybaud commented Jun 8, 2020

Indeed I'll share a common settings file to import for Intellij IDEA users to properly make changes without braking the format of every files. (tbd where in the project)

}
.filter {
!(NGSILD_PROPERTIES_CORE_MEMBERS.plus(NGSILD_RELATIONSHIPS_CORE_MEMBERS)).contains(it.key) &&
.mapValues {
Copy link
Member

@bobeal bobeal Jun 8, 2020

Choose a reason for hiding this comment

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

I find weird this extra indent. I guess it is considered like a continuation indent but ...

Copy link
Member

Choose a reason for hiding this comment

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

I would be more comfortable with something like:

values
    .map {
    }
    .filter {
    }

Copy link
Member

Choose a reason for hiding this comment

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

Rather than

values.map {
}
    .filter {
    }

Copy link
Contributor Author

Choose a reason for hiding this comment

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

I don't think continuation indent applies to braces...

I think expected result is :

values.filterValues {
    it[0] is Map<*, *>
}.mapValues {
    expandValueAsMap(it.value)
}.filter {
    !(NGSILD_PROPERTIES_CORE_MEMBERS.plus(NGSILD_RELATIONSHIPS_CORE_MEMBERS)).contains(it.key) &&
        isAttributeOfType(it.value, NGSILD_PROPERTY_TYPE)
}.forEach { entry ->
    if (!neo4jRepository.hasPropertyOfName(subject.id, entry.key))
        throw BadRequestDataException("Property ${entry.key.extractShortTypeFromExpanded()} does not exist")
    updatePropertyValues(subject.id, entry.key, entry.value)
}

But there were a newline placed in first line..
What do you think ?

Copy link
Member

Choose a reason for hiding this comment

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

I'm fine with your expected result!

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Fixed in d6d00ee

@bobeal
Copy link
Member

bobeal commented Jun 8, 2020

Indeed I'll share a common settings file to import for Intellij IDEA users to properly make changes without braking the format of every files. (tbd where in the project)

Add this in the PR (with instructions in the README)

@vraybaud
Copy link
Contributor Author

vraybaud commented Jun 9, 2020

Indeed I'll share a common settings file to import for Intellij IDEA users to properly make changes without braking the format of every files. (tbd where in the project)

Add this in the PR (with instructions in the README)

I will add instructions in the README for this PR but we should do an issue (not priority) to create a CONTRIBUTING.md

@bobeal
Copy link
Member

bobeal commented Jun 9, 2020

Indeed I'll share a common settings file to import for Intellij IDEA users to properly make changes without braking the format of every files. (tbd where in the project)

Add this in the PR (with instructions in the README)

I will add instructions in the README for this PR but we should do an issue (not priority) to create a CONTRIBUTING.md

Yep, create the issue.

@vraybaud vraybaud force-pushed the refactoring/format-all-code branch from 6f8032b to 85247f2 Compare June 9, 2020 10:17
@vraybaud vraybaud linked an issue Jun 9, 2020 that may be closed by this pull request
@vraybaud vraybaud force-pushed the refactoring/format-all-code branch 4 times, most recently from 127a90c to 623edc0 Compare June 9, 2020 14:20
@vraybaud vraybaud force-pushed the refactoring/format-all-code branch from 623edc0 to 19f61c9 Compare June 9, 2020 14:21
@vraybaud vraybaud merged commit 6804f01 into develop Jun 9, 2020
@vraybaud vraybaud deleted the refactoring/format-all-code branch June 9, 2020 14:22
@github-actions github-actions bot locked and limited conversation to collaborators Jun 9, 2020
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.

Format all files aligned with ktlint and share the format settings
3 participants