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.

Sign up for GitHub

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

Jump to bottom

KTOR 1344 - Unified pre-typed routes #2211

Merged
merged 5 commits into from
Mar 10, 2021
Merged

KTOR 1344 - Unified pre-typed routes #2211

merged 5 commits into from
Mar 10, 2021

Conversation

hfhbd
Copy link
Contributor

@hfhbd hfhbd commented Nov 20, 2020

Subsystem
Server - Routing

Motivation
Currently, only post has a typed overload. KTOR-1344

Solution
Added a typed overload to put and patch as well

@cy6erGn0m cy6erGn0m self-requested a review November 29, 2020 09:49
@cy6erGn0m cy6erGn0m self-assigned this Nov 29, 2020
cy6erGn0m
cy6erGn0m suggested changes Nov 29, 2020
View reviewed changes
Copy link
Contributor

@cy6erGn0m cy6erGn0m left a comment

Choose a reason for hiding this comment

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

LGTM except for bytecode duplication

ktor-server/ktor-server-tests/jvm/test/io/ktor/tests/server/routing/RoutingProcessingTest.kt Outdated Show resolved Hide resolved
ktor-server/ktor-server-tests/jvm/test/io/ktor/tests/server/routing/RoutingProcessingTest.kt Outdated Show resolved Hide resolved
ktor-server/ktor-server-core/jvm/src/io/ktor/routing/RoutingBuilder.kt Outdated Show resolved Hide resolved
@hfhbd hfhbd requested a review from cy6erGn0m November 29, 2020 13:43
@cy6erGn0m cy6erGn0m added this to the 1.6.0 milestone Jan 22, 2021
@cy6erGn0m
Copy link
Contributor

cy6erGn0m commented Feb 11, 2021 •
edited
Loading

@rsinukov This generally looks fine but I feel these function may/will clash with locations functions so I wonder if we can somehow workaround it? The functions contributed seems useful but I don't know how do we avoid ambiguity. What do you think?

@rsinukov
Copy link
Contributor

@cy6erGn0m it looks ok for me. Locations builders and these ones are in different packages, so users can always specify what function they want to use. We already have this for post and we never had any complaints about it.

rsinukov
rsinukov suggested changes Feb 11, 2021
View reviewed changes
Copy link
Contributor

@rsinukov rsinukov left a comment

Choose a reason for hiding this comment

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

Please run ./gradlew apiDump and change the target branch to 1.6.0

@hfhbd hfhbd changed the base branch from master to 1.6.0-eap February 11, 2021 14:05
@hfhbd
Copy link
Contributor Author

hfhbd commented Feb 11, 2021

@rsinukov Changed. apiDump did not update my changes, because all functions are inline reified.

@cy6erGn0m
Copy link
Contributor

@hfhbd Ok, let's proceed with merge. Could you please rebase it and skip irrelevant commits? Notice the base branch 1.6.0-eap

@hfhbd
Copy link
Contributor Author

hfhbd commented Mar 4, 2021 •
edited
Loading

Dont merge, one second, I want to fix my username

@hfhbd
Copy link
Contributor Author

hfhbd commented Mar 4, 2021

Okay, I don't care about the username
@cy6erGn0m rebased

@cy6erGn0m cy6erGn0m merged commit 12b3e5e into ktorio:1.6.0-eap Mar 10, 2021
@hfhbd hfhbd deleted the hfhbd/ktor-1344 branch March 10, 2021 14:56
cy6erGn0m pushed a commit that referenced this pull request Mar 10, 2021
@hfhbd
KTOR 1344 Add unified pre-typed routes (#2211)
9fc4707 
* KTOR-1344 Add failing test

* KTOR-1344 Add put and post typed overload

* KTOR-1344 Split typed functions into two, one with path and one without

* KTOR-1344 Refactor test

* KTOR-1344 Remove path from comments

Co-authored-by: hfhbd <[email protected]>
e5l pushed a commit that referenced this pull request Mar 17, 2021
@hfhbd @e5l
KTOR 1344 Add unified pre-typed routes (#2211)
e2f8af5 
* KTOR-1344 Add failing test

* KTOR-1344 Add put and post typed overload

* KTOR-1344 Split typed functions into two, one with path and one without

* KTOR-1344 Refactor test

* KTOR-1344 Remove path from comments

Co-authored-by: hfhbd <[email protected]>
e5l pushed a commit that referenced this pull request Apr 6, 2021
@hfhbd @e5l
KTOR 1344 Add unified pre-typed routes (#2211)
c20c741 
* KTOR-1344 Add failing test

* KTOR-1344 Add put and post typed overload

* KTOR-1344 Split typed functions into two, one with path and one without

* KTOR-1344 Refactor test

* KTOR-1344 Remove path from comments

Co-authored-by: hfhbd <[email protected]>
rsinukov pushed a commit that referenced this pull request Apr 7, 2021
@hfhbd @rsinukov
KTOR 1344 Add unified pre-typed routes (#2211)
5911c5e 
* KTOR-1344 Add failing test

* KTOR-1344 Add put and post typed overload

* KTOR-1344 Split typed functions into two, one with path and one without

* KTOR-1344 Refactor test

* KTOR-1344 Remove path from comments

Co-authored-by: hfhbd <[email protected]>
e5l pushed a commit that referenced this pull request Apr 8, 2021
@hfhbd @e5l
KTOR 1344 Add unified pre-typed routes (#2211)
45af024 
* KTOR-1344 Add failing test

* KTOR-1344 Add put and post typed overload

* KTOR-1344 Split typed functions into two, one with path and one without

* KTOR-1344 Refactor test

* KTOR-1344 Remove path from comments

Co-authored-by: hfhbd <[email protected]>
rsinukov pushed a commit that referenced this pull request Apr 9, 2021
@hfhbd @rsinukov
KTOR 1344 Add unified pre-typed routes (#2211)
3cfb4ea 
* KTOR-1344 Add failing test

* KTOR-1344 Add put and post typed overload

* KTOR-1344 Split typed functions into two, one with path and one without

* KTOR-1344 Refactor test

* KTOR-1344 Remove path from comments

Co-authored-by: hfhbd <[email protected]>
e5l pushed a commit that referenced this pull request Apr 12, 2021
@hfhbd @e5l
KTOR 1344 Add unified pre-typed routes (#2211)
db23645 
* KTOR-1344 Add failing test

* KTOR-1344 Add put and post typed overload

* KTOR-1344 Split typed functions into two, one with path and one without

* KTOR-1344 Refactor test

* KTOR-1344 Remove path from comments

Co-authored-by: hfhbd <[email protected]>
e5l pushed a commit that referenced this pull request Apr 28, 2021
@hfhbd @e5l
KTOR 1344 Add unified pre-typed routes (#2211)
921519c 
* KTOR-1344 Add failing test

* KTOR-1344 Add put and post typed overload

* KTOR-1344 Split typed functions into two, one with path and one without

* KTOR-1344 Refactor test

* KTOR-1344 Remove path from comments

Co-authored-by: hfhbd <[email protected]>
e5l pushed a commit that referenced this pull request Apr 29, 2021
@hfhbd @e5l
KTOR 1344 Add unified pre-typed routes (#2211)
4f69cc9 
* KTOR-1344 Add failing test

* KTOR-1344 Add put and post typed overload

* KTOR-1344 Split typed functions into two, one with path and one without

* KTOR-1344 Refactor test

* KTOR-1344 Remove path from comments

Co-authored-by: hfhbd <[email protected]>
cy6erGn0m pushed a commit that referenced this pull request Apr 30, 2021
@hfhbd
KTOR 1344 Add unified pre-typed routes (#2211)
20da797 
* KTOR-1344 Add failing test

* KTOR-1344 Add put and post typed overload

* KTOR-1344 Split typed functions into two, one with path and one without

* KTOR-1344 Refactor test

* KTOR-1344 Remove path from comments

Co-authored-by: hfhbd <[email protected]>
hfhbd added a commit to hfhbd/ktor that referenced this pull request Jun 7, 2021
@hfhbd
KTOR 1344 Add unified pre-typed routes (ktorio#2211)
4087fe0 
* KTOR-1344 Add failing test

* KTOR-1344 Add put and post typed overload

* KTOR-1344 Split typed functions into two, one with path and one without

* KTOR-1344 Refactor test

* KTOR-1344 Remove path from comments

Co-authored-by: hfhbd <[email protected]>
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Reviewers

@cy6erGn0m cy6erGn0m cy6erGn0m approved these changes

@rsinukov rsinukov rsinukov approved these changes

Assignees

@cy6erGn0m cy6erGn0m

Labels
Server
Projects
None yet
Milestone
1.6.0
Development

Successfully merging this pull request may close these issues.

None yet
3 participants
@hfhbd @cy6erGn0m @rsinukov