add _port to url_for, update routing doc #1406

lixxu · 2018-11-08T03:21:25Z

No description provided.

vltr · 2018-11-08T11:35:51Z

sanic/app.py

@@ -690,6 +698,16 @@ def url_for(self, view_name: str, **kwargs):

        # parse the remainder of the keyword arguments into a querystring
        query_string = urlencode(kwargs, doseq=True) if kwargs else ""
+
+        if external and port:
+            port = int(port)


I know this may sound silly, but shouldn't we wrap this call with a try/except in case int raises a ValueError or TypeError? Just in case someone tries to pass some random string or some other weird object to the _port argument or SERVER_PORT configuration key? At least we will prevent any issues in the future with this ...

yes, sounds reasonable.

Great, thanks @lixxu !

I actually like this more as it was, makes it impossible to use it wrong. the try/except logic lets you pass a nonsense port and doesn't even complain

vltr · 2018-11-08T12:03:35Z

sanic/app.py

+            try:
+                port = int(port)
+            except (ValueError, TypeError):
+                pass  # just ignore it


Yeah, I think we can just ignore this (for now). If someone complains in the future, we can perhaps just add a logger.warn call here and that should suffice.

Don't know why lint check failed.

@lixxu try to run black to format the code (in the root of the repo):

$ black ./sanic

oops, 69 files reformatted, 19 files left unchanged. (almost all are single quote replaced by double quotes). Should I commit these changes? a lot files, may conflict with other PRs.

Have you rebased with the last master?

sorry, I ran the cmd outside of sanic folder, now only the 2 files updated. :)

Ohh, that makes sense 😄

TLDR; Fail fast and fail loud.
I saw that @lixxu had already implemented it first without try:except then they were asked to put it in a try:except block. In this case, I believe failing loudly would have better point that failing silently here. A user might wrongly pass some object that can't be converted into integer and this function will fail silently.

codecov-io · 2018-11-08T12:24:49Z

Codecov Report

Merging #1406 into master will decrease coverage by 0.36%.
The diff coverage is 58.82%.

@@            Coverage Diff             @@
##           master    #1406      +/-   ##
==========================================
- Coverage   91.35%   90.99%   -0.37%     
==========================================
  Files          18       18              
  Lines        1781     1787       +6     
  Branches      337      340       +3     
==========================================
- Hits         1627     1626       -1     
- Misses        130      135       +5     
- Partials       24       26       +2

Impacted Files	Coverage Δ
sanic/testing.py	`92.5% <50%> (-2.8%)`	⬇️
sanic/app.py	`91.07% <61.53%> (-1.18%)`	⬇️
sanic/router.py	`95.89% <0%> (+0.45%)`	⬆️

Continue to review full report at Codecov.

Legend - Click here to learn more
Δ = absolute <relative> (impact), ø = not affected, ? = missing data
Powered by Codecov. Last update d581315...e6dd8cb. Read the comment docs.

ahopkins

If port 21 is included, why not 22?

lixxu · 2018-11-08T12:29:16Z

22 is used for ssh, and I think (maybe wrong) it may have no chance to generate urls for ssh. Or you means for sftp?

ahopkins · 2018-11-08T13:05:32Z

Yes, sftp, which ... as far as I know is just a cover for ssh on port 22.

Regardless, is that even something that we want to be supporting? Why have any ftp at all? Am I missing something?

vltr · 2018-11-08T13:09:48Z

I agree with @ahopkins . We should stick to http and https since Sanic is a web framework [...]

lixxu · 2018-11-08T13:18:26Z

ftp and ftps are from testing.py, they are there maybe in case that someone use sanic to write ftp/ftps server?

vltr · 2018-11-08T13:29:56Z

That would have the need of a completely different protocol implementation ... As well as a lot of FTP specific features like mapping all the commands, perhaps build a proper FS integration and etc, etc ... I think it'll be too much work to make it run on top of Sanic - unless someone already had the trouble to do something like that (and I would be really impressed).

If I needed (today) to implement my own custom FTP server, I would go with Twisted or aioftp. No need to reinvent the wheel ...

lixxu · 2018-11-08T13:33:44Z

so, should remove ftp stuff from url_for?

vltr · 2018-11-08T13:44:24Z

IMO it makes more sense (inside Sanic) to remove ftp from the "port normalization" part of the function.

harshanarayana · 2018-11-08T13:48:40Z

sanic/app.py

+
+        port = kwargs.pop("_port", None)
+        if port is None:
+            port = self.config.get("SERVER_PORT", "")


Wouldn't it be much simpler to merge all of these into a single line and let python handle it rather than the conditional branching?

port = kwargs.pop('_port', self.config.get('SERVER_PORT', ''))

single line format will not get the value from configuration if someone passes _port=None as parameter.

@lixxu
The following should give you the same output.

port = kwargs.pop('_port', self.config.get('SERVER_PORT', '')) or ''

I mentioned this since I've seen a great performance improvement with the removal of each if-else construct. I know this doesn't add too much overhead but still felt like a good idea.

PS: Yes, I see it now. The grave i've been digging for myself. 😛

Just FYI,

>>> timeit.timeit(stmt=lookup, number=1000000) 0.472626347996993 >>> timeit.timeit(stmt=cond, number=1000000) 0.5024505830078851

ok, updated for it. but I made the commits history ugly. :( wonder how to squash it again.

ahopkins · 2018-11-08T15:26:56Z

@lixxu, can you also squash these commits?

lixxu · 2018-11-08T20:46:41Z

sorry, I made the commits ugly. Is there anyway to fix it? :(

harshanarayana · 2018-11-09T04:27:43Z

@lixxu git rebase -i commit-id-to-start

When rebase is done it will prompt you to edit the messages

lixxu · 2018-11-09T04:52:16Z

tks @harshanarayana, seems worked (with force pushed :( ).

harshanarayana · 2018-11-09T05:15:58Z

@lixxu That's expected. Since you rebased it, now the commit IDs in github and the one you are trying to push are all wibly wobly. So you have to use -f more or less always when you use git rebase commands.

sjsadowski · 2018-11-12T15:12:31Z

I'm tagging this for 19.03 since it is a change, not a fix.

lixxu · 2018-11-12T20:18:07Z

the issue #1380 was tagged for 18.12 lts.

sjsadowski · 2018-11-12T21:47:24Z

That is because #1380 is pointing out a bug. This fixes a bug, yes, but it also adds functionality.

abuckenheimer · 2018-11-17T19:28:06Z

sanic/app.py

                netloc = netloc.split("://", 1)[-1]

+            if not netloc:
+                scheme = ""
+                external = False


the branching here is getting a bit complicated, I'd maybe suggest something like

scheme = re.match( r"(?P<scheme>https?)?(?(scheme)://)(?P<netloc>\w+)", kwargs.pop("_scheme", "") ).groupdict() if scheme: scheme, netloc = scheme['scheme'], scheme['netloc'] else: scheme, netloc = ""

but we can always address this seperately

yunstanford · 2018-11-30T09:52:20Z

This PR looks like out of date, could you update it ?

yunstanford

Could you update this outdated branch ?

lixxu · 2018-12-05T06:15:31Z

Sorry, how to update it?

MuhistheHolvi · 2018-12-05T07:37:06Z

@lixxu You can either merge master to your local branch and then merge or you can rebase your branch onto master branch. You need to resolve the conflicts as shown in github.

sjsadowski · 2019-03-03T22:40:41Z

@lixxu can you update your branch?

lixxu · 2019-03-04T02:12:11Z

@lixxu can you update your branch?

updated, please review.

sjsadowski · 2019-03-04T16:04:11Z

@lixxu can you back that out, do a squash and merge or rebase instead? This is gonna be a huge amount of clutter in the commits to merge back in.

* enable blueprint group middleware support This commit will enable the users to implement a middleware at the blueprint group level whereby enforcing the middleware automatically to each of the available Blueprints that are part of the group. This will eanble a simple way in which a certain set of common features and criteria can be enforced on a Blueprint group. i.e. authentication and authorization This commit will address the feature request raised as part of Issue sanic-org#1386 Signed-off-by: Harsha Narayana <[email protected]> * enable indexing of BlueprintGroup object Signed-off-by: Harsha Narayana <[email protected]> * rename blueprint group file to fix spelling error Signed-off-by: Harsha Narayana <[email protected]> * add documentation and additional unit tests Signed-off-by: Harsha Narayana <[email protected]> * cleanup and optimize headers in unit test file Signed-off-by: Harsha Narayana <[email protected]> * fix Bluprint Group iteratable method Signed-off-by: Harsha Narayana <[email protected]> * add additional unit test to check StopIteration condition Signed-off-by: Harsha Narayana <[email protected]> * cleanup iter protocol implemenation for blueprint group and add slots Signed-off-by: Harsha Narayana <[email protected]> * fix blueprint group middleware invocation identification Signed-off-by: Harsha Narayana <[email protected]> * feat: enable list behavior on blueprint group object and use append instead of properly to add blueprint to group Signed-off-by: Harsha Narayana <[email protected]> add port to url_for, and update doc for it, reset scheme and external if server not specified in url_for add port to url_for, and update doc for it, reset scheme and external if server not specified in url_for Revert "add port to url_for, and update doc for it, reset scheme and external if server not specified in url_for" This reverts commit b073dfa.

lixxu · 2019-03-05T05:19:39Z

oops, I lost the code lines (sorry, not familiar with squash, rebase), how to find them back?

lixxu mentioned this pull request Nov 8, 2018

Port not provided in URLs produced with url_for #1380

Closed

vltr requested changes Nov 8, 2018

View reviewed changes

vltr reviewed Nov 8, 2018

View reviewed changes

ahopkins reviewed Nov 8, 2018

View reviewed changes

harshanarayana reviewed Nov 8, 2018

View reviewed changes

lixxu force-pushed the master branch from 603c985 to 53531bf Compare November 8, 2018 20:19

lixxu force-pushed the master branch from 9d46242 to db76b0b Compare November 9, 2018 04:49

sjsadowski added this to the 19.03 milestone Nov 12, 2018

abuckenheimer previously approved these changes Nov 17, 2018

View reviewed changes

yunstanford requested changes Dec 5, 2018

View reviewed changes

sjsadowski closed this Mar 3, 2019

sjsadowski reopened this Mar 3, 2019

lixxu dismissed abuckenheimer’s stale review via b2bedfd March 4, 2019 02:11

lixxu force-pushed the master branch from b2bedfd to e6dd8cb Compare March 5, 2019 04:43

lixxu closed this Mar 17, 2019

add _port to url_for, update routing doc #1406

add _port to url_for, update routing doc #1406

Conversation

lixxu commented Nov 8, 2018

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

lixxu Nov 8, 2018 • edited Loading

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

codecov-io commented Nov 8, 2018 • edited by codecov bot Loading

Codecov Report

ahopkins left a comment

Choose a reason for hiding this comment

lixxu commented Nov 8, 2018

ahopkins commented Nov 8, 2018 • edited Loading

vltr commented Nov 8, 2018

lixxu commented Nov 8, 2018

vltr commented Nov 8, 2018

lixxu commented Nov 8, 2018

vltr commented Nov 8, 2018

Choose a reason for hiding this comment

Choose a reason for hiding this comment

harshanarayana Nov 8, 2018 • edited Loading

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

ahopkins commented Nov 8, 2018

lixxu commented Nov 8, 2018

harshanarayana commented Nov 9, 2018

lixxu commented Nov 9, 2018

harshanarayana commented Nov 9, 2018

sjsadowski commented Nov 12, 2018

lixxu commented Nov 12, 2018

sjsadowski commented Nov 12, 2018

Choose a reason for hiding this comment

yunstanford commented Nov 30, 2018

yunstanford left a comment

Choose a reason for hiding this comment

lixxu commented Dec 5, 2018

MuhistheHolvi commented Dec 5, 2018

sjsadowski commented Mar 3, 2019

lixxu commented Mar 4, 2019

sjsadowski commented Mar 4, 2019

lixxu commented Mar 5, 2019

lixxu Nov 8, 2018 •

edited

Loading

codecov-io commented Nov 8, 2018 •

edited by codecov bot

Loading

ahopkins commented Nov 8, 2018 •

edited

Loading

harshanarayana Nov 8, 2018 •

edited

Loading