Skip to content
This repository was archived by the owner on Dec 19, 2023. It is now read-only.

Move milliseconds and seconds to duration #647

Merged
merged 1 commit into from
Jul 28, 2021
Merged

Move milliseconds and seconds to duration #647

merged 1 commit into from
Jul 28, 2021

Conversation

m1ngyuan
Copy link

@m1ngyuan m1ngyuan commented Jun 5, 2021 •
edited
Loading

fixes #645 move milliseconds and seconds to duration

@m1ngyuan m1ngyuan marked this pull request as ready for review June 5, 2021 09:37
@m1ngyuan m1ngyuan changed the title fixes(#645): Move milliseconds and seconds to duration Move milliseconds and seconds to duration Jun 5, 2021
@m1ngyuan m1ngyuan marked this pull request as draft June 5, 2021 09:55
@m1ngyuan m1ngyuan marked this pull request as ready for review June 5, 2021 10:21
@m1ngyuan m1ngyuan marked this pull request as draft June 5, 2021 11:00
@m1ngyuan m1ngyuan marked this pull request as ready for review June 5, 2021 11:21
@BlasiusSecundus
Copy link

Also, the commit type should be feat. Since this is actually adding new / modifying existing features of the libary.

@m1ngyuan m1ngyuan requested a review from BlasiusSecundus June 8, 2021 20:28
@m1ngyuan m1ngyuan requested a review from BlasiusSecundus June 10, 2021 01:28
@BlasiusSecundus
Copy link

Please squash the two commits, in this case I think they should be one.

BlasiusSecundus
BlasiusSecundus reviewed Jun 17, 2021
View reviewed changes
...hql-spring-boot-test/src/main/java/com/graphql/spring/boot/test/GraphQLTestSubscription.java Outdated
* does not have any expectation regarding the number of messages. The subscription will be
* stopped after the time elapsed.
*
* @param timeToWait the time to wait, in durations
Copy link

Choose a reason for hiding this comment

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

"in durations" sounds strange again. Simply "time to wait" seems enoungh, unit of measurement is now part of the input.

Copy link
Author

Choose a reason for hiding this comment

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

I've dealt with it.

BlasiusSecundus
BlasiusSecundus reviewed Jun 17, 2021
View reviewed changes
...figure/src/main/java/graphql/kickstart/autoconfigure/web/servlet/AsyncServletProperties.java Outdated
/**
* Asynchronous execution timeout. If a duration suffix is not specified, millisecond will be used.
*/
@NonNull
Copy link

Choose a reason for hiding this comment

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

I would drop @NonNull for now, and return to the validation part in a separate PR.

Copy link
Author

Choose a reason for hiding this comment

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

I've dealt with it.

@BlasiusSecundus
Copy link

Thanks for the contribution, looks good for me. Added some final comments, mostly cosmetic things. I noticed that in the javadoc part there are several "in durations", which sounds strange. (Possibly a result of replacing millisecond with duration?)

@BlasiusSecundus BlasiusSecundus added this to the 12.0.0 milestone Jun 17, 2021
@BlasiusSecundus
Copy link

Added this to the next milestone. I see @oliemansm added another configuration-related issue (#232) - so maybe it would be a good time to review config properties as a whole:

  • add property validation where it could be useful
  • maybe there is room for smaller adjustments/improvements like this one

@m1ngyuan
Copy link
Author

When I submitted PR, I also found another questionable code, was it intentional or typo

https://github.com/graphql-java-kickstart/graphql-spring-boot/blob/master/graphql-spring-boot-autoconfigure/src/main/java/graphql/kickstart/autoconfigure/web/servlet/GraphQLWebAutoConfiguration.java#L319

@BlasiusSecundus
Copy link

BlasiusSecundus commented Jun 22, 2021 •
edited
Loading

Yes, this definitely looks like a mistake. It should be either min or max (max is currently overriding min, and I would assume max was intended originally).

BlasiusSecundus
BlasiusSecundus reviewed Jun 22, 2021
View reviewed changes
...hql-spring-boot-test/src/main/java/com/graphql/spring/boot/test/GraphQLTestSubscription.java Outdated
/**
* Waits a specified amount of time and asserts that no responses were received during that time.
*
* @param timeToWait time to wait, in durations.
Copy link

Choose a reason for hiding this comment

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

"in durations"

BlasiusSecundus
BlasiusSecundus reviewed Jun 22, 2021
View reviewed changes
...hql-spring-boot-test/src/main/java/com/graphql/spring/boot/test/GraphQLTestSubscription.java Outdated
* Waits a specified amount of time and asserts that no responses were received during that time.
* The subscription will be stopped afterwards.
*
* @param timeToWait time to wait, in durations.
Copy link

Choose a reason for hiding this comment

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

"in durations"

@oliemansm oliemansm merged commit 4d5fb1f into graphql-java-kickstart:master Jul 28, 2021
@m1ngyuan m1ngyuan deleted the use_duration branch July 29, 2021 05:13
Sign up for free to subscribe to this conversation on GitHub. Already have an account? Sign in.
Reviewers

@BlasiusSecundus BlasiusSecundus BlasiusSecundus approved these changes

Assignees
No one assigned
Labels
None yet
Projects
None yet
Milestone
12.0.0
Development

Successfully merging this pull request may close these issues.

Move milliseconds and seconds to duration
3 participants
@m1ngyuan @BlasiusSecundus @oliemansm