docs(user): add query strings tutorial #2239

bssyousefi · 2024-07-12T15:12:32Z

Summary of Changes

Fill in the placeholder for Query Strings in the documentation. The placeholder only existed on WSGI tutorial, so the proposed change is just about that part of the documentation.

Related Issues

Fixes #475.

Pull Request Checklist

This is just a reminder about the most common mistakes. Please make sure that you tick all appropriate boxes. But please read our contribution guide at least once; it will save you a few review cycles!

If an item doesn't apply to your pull request, check it anyway to make it apparent that there's nothing to do.

If you have any questions to any of the points above, just submit and ask! This checklist is here to help you, not to deter you from contributing!

PR template inspired by the attrs project.

CaselIT

Thanks for the PR, great work!

I've left a few suggestions

CaselIT · 2024-07-13T08:13:18Z

docs/user/tutorial.rst

+            images = self._image_store.list(max_size)
+            doc = {
+                'images': [
+                    {'href': '/images/'+image} for image in images


Suggested change

{'href': '/images/'+image} for image in images

{'href': '/images/' + image} for image in images

CaselIT · 2024-07-13T08:17:12Z

docs/user/tutorial.rst

+        def __init__(self, image_store):
+            self._image_store = image_store
+
+        def on_get(self, req, resp):


This version should be a static one like the above that serializes to msgpack, since we refer to it as "static"

My bad 🥲 . You're totally right.

CaselIT · 2024-07-13T08:17:24Z

docs/user/tutorial.rst

+            images = self._image_store.list(max_size)
+            doc = {
+                'images': [
+                    {'href': '/images/'+image} for image in images


Suggested change

{'href': '/images/'+image} for image in images

{'href': '/images/' + image} for image in images

CaselIT · 2024-07-13T08:23:26Z

docs/user/tutorial.rst

+            ]
+            return images
+
+As you can see the method ``list`` has been added to ``ImageStore`` in order to return list of available images smaller than ``max_size`` unless it is not zero, in which case it will behave like there was no predicament of image size.


maybe we could use -1 for the "defaut" here?

CaselIT · 2024-07-13T08:30:53Z

docs/user/tutorial.rst

+            return images
+
+As you can see the method ``list`` has been added to ``ImageStore`` in order to return list of available images smaller than ``max_size`` unless it is not zero, in which case it will behave like there was no predicament of image size.
+Let's try to save some binary data as images in the service and then try to retrieve their list. Execute the following commands in order to make 3 files as images with different sizes.


since these are not really images, we could use "images" here and maybe say something like

Execute the following commands in order to simulate the creation of 3 files as images with different sizes. While these are not valid PNG files, they will work for this tutorial.

CaselIT · 2024-07-13T08:33:45Z

docs/user/tutorial.rst

+            self._image_store = image_store
+
+        def on_get(self, req, resp):
+            max_size = int(req.params.get("maxsize", 0))


I think it would be useful to showcase get_param_as_int here, so we could do

Suggested change

max_size = int(req.params.get("maxsize", 0))

max_size = int(req.get_param_as_int("maxsize", default=0, min_value=1))

(I've added a min value but we may omit it too)

bssyousefi · 2024-07-13T23:05:56Z

I have a question regarding a part of the code.

 _IMAGE_NAME_PATTERN = re.compile(
            '[0-9a-f]{8}-[0-9a-f]{4}-[0-9a-f]{4}-[0-9a-f]{4}-[0-9a-f]{12}\.[a-z]{2,4}$'
        )

which I used from previous parts of the docs.
the string used as the pattern will raise error in python. I guess we should use one of the following ways

 _IMAGE_NAME_PATTERN = re.compile(
            '[0-9a-f]{8}-[0-9a-f]{4}-[0-9a-f]{4}-[0-9a-f]{4}-[0-9a-f]{12}.[a-z]{2,4}$'
        )
 _IMAGE_NAME_PATTERN = re.compile(
            r'[0-9a-f]{8}-[0-9a-f]{4}-[0-9a-f]{4}-[0-9a-f]{4}-[0-9a-f]{12}\.[a-z]{2,4}$'
        )

However, I didn't try to fix it here and I thought that maybe I should first report it as an issue and then resolve it in another PR. Am I right or should I try the fix on this PR since it's a small change?

codecov · 2024-07-18T19:04:50Z

Codecov Report

All modified and coverable lines are covered by tests ✅

Project coverage is 100.00%. Comparing base (f1a7f2d) to head (7ae761e).
Report is 1 commits behind head on master.

Additional details and impacted files

@@            Coverage Diff            @@
##            master     #2239   +/-   ##
=========================================
  Coverage   100.00%   100.00%           
=========================================
  Files           64        64           
  Lines         7726      7726           
  Branches      1071      1071           
=========================================
  Hits          7726      7726

☔ View full report in Codecov by Sentry.
📢 Have feedback on the report? Share it here.

vytas7

Thanks for this contribution, looks good for the most of it.

We just need to clean up things a bit before merging (see my comments inline, as well as @CaselIT's remarks).

docs/user/tutorial.rst

vytas7 · 2024-08-25T14:49:39Z

docs/user/tutorial.rst

+
+Let's go back to the ``on_get`` method and create a dynamic response. We can use query strings to set maximum image size and get the list of all images smaller than the specified value. We will use method ``get_param_as_int`` to set a default value of ``-1`` in case no ``maxsize`` query string was provided and also to enable a minimum value validation.
+
+.. code:: python


Don't we need to update any example files on disk? And write tests for them too?

(See also: #2247.)

Sure, I'll look into this.

@vytas7, I see that a sample usage of query params is being used in the example file for wsgi.

falcon/examples/things_advanced.py

Line 158 in 65aa7f6

limit = req.get_param_as_int('limit') or 50

Are we looking for a more descriptive example?

Hi again, no, sorry I was not very clear!

No, it has nothing to do with things_advanced.py; what I meant was that we are tracking tutorial files in the Git tree as well, we even have some rudimentary tests for them. In this case it looks like you might need to update images.py with your additions, and potentially even revise how the file develops further in this tutorial.

bssyousefi · 2024-08-26T03:34:16Z

I have a question regarding a part of the code.
 _IMAGE_NAME_PATTERN = re.compile(
            '[0-9a-f]{8}-[0-9a-f]{4}-[0-9a-f]{4}-[0-9a-f]{4}-[0-9a-f]{12}\.[a-z]{2,4}$'
        )
which I used from previous parts of the docs. the string used as the pattern will raise error in python. I guess we should use one of the following ways
 _IMAGE_NAME_PATTERN = re.compile(
            '[0-9a-f]{8}-[0-9a-f]{4}-[0-9a-f]{4}-[0-9a-f]{4}-[0-9a-f]{12}.[a-z]{2,4}$'
        )
 _IMAGE_NAME_PATTERN = re.compile(
            r'[0-9a-f]{8}-[0-9a-f]{4}-[0-9a-f]{4}-[0-9a-f]{4}-[0-9a-f]{12}\.[a-z]{2,4}$'
        )
However, I didn't try to fix it here and I thought that maybe I should first report it as an issue and then resolve it in another PR. Am I right or should I try the fix on this PR since it's a small change?

@vytas7 Would you please also help me on this?

vytas7 · 2024-08-30T15:53:14Z

docs/user/tutorial.rst

+
+Let's go back to the ``on_get`` method and create a dynamic response. We can use query strings to set maximum image size and get the list of all images smaller than the specified value. We will use method ``get_param_as_int`` to set a default value of ``-1`` in case no ``maxsize`` query string was provided and also to enable a minimum value validation.
+
+.. code:: python


Hi again, no, sorry I was not very clear!

No, it has nothing to do with things_advanced.py; what I meant was that we are tracking tutorial files in the Git tree as well, we even have some rudimentary tests for them. In this case it looks like you might need to update images.py with your additions, and potentially even revise how the file develops further in this tutorial.

bssyousefi · 2024-09-23T17:59:37Z

@vytas7 I'm very sorry for the very long delay. I mistakenly thought that all the tests were passed and I didn't got time to get back to the project since I was a little bit busy.
Thanks for you patience.
I will be more available and responsible from now on.

vytas7 · 2024-09-23T18:00:57Z

No worries, @bssyousefi 🙂
We are now focusing on getting 4.0 out the door, so your PR is not urgent. But it will be nice to have it done when ready.

bssyousefi · 2024-09-23T18:02:25Z

I guess it's ready. I've updated the tests for the example look

CaselIT

Nice work, just a couple of minor suggestions

examples/look/look/images.py

examples/look/tests/test_app.py

bssyousefi · 2024-11-06T20:49:32Z

Hi,
Is there any issue blocking this PR?

vytas7 · 2024-11-06T20:51:28Z

Hi @bssyousefi, and thanks for bringing this up. No, I don't think so, I just need to review this again.

vytas7

Let's merge this as-is, thanks for this improvement @bssyousefi!

It seems that there are a couple of minor inconsistencies, but that could be left as an exercise for the reader. Ultimately, we should move to the approach suggested in #2247, otherwise it is hard to keep track of all the minutiae by hand.

add query strings to docs/user/tutorial

bbacdcb

CaselIT requested changes Jul 13, 2024

View reviewed changes

fix some parts in the docs

3be386a

vytas7 changed the title ~~add query strings to docs/user/tutorial~~ docs(user): add query strings tutorial Jul 17, 2024

bssyousefi requested a review from CaselIT July 19, 2024 11:59

vytas7 added 3 commits July 21, 2024 16:51

Merge branch 'master' into Issue-475/add-docs-query-strings

c0b0863

Merge branch 'master' into Issue-475/add-docs-query-strings

e979219

Merge branch 'master' into Issue-475/add-docs-query-strings

a539420

vytas7 requested changes Aug 25, 2024

View reviewed changes

vytas7 and others added 2 commits August 25, 2024 16:50

Merge branch 'master' into Issue-475/add-docs-query-strings

8d87546

shorten rst lines to around 80

d599b1e

bssyousefi requested a review from vytas7 August 27, 2024 02:20

CaselIT previously approved these changes Aug 27, 2024

View reviewed changes

vytas7 added 2 commits August 27, 2024 19:06

Merge branch 'master' into Issue-475/add-docs-query-strings

e5f0d54

Merge branch 'master' into Issue-475/add-docs-query-strings

ae44d2b

vytas7 requested changes Aug 30, 2024

View reviewed changes

update examples/look and tests

636ac6c

bssyousefi dismissed CaselIT’s stale review via 636ac6c September 1, 2024 16:26

bssyousefi requested a review from vytas7 September 1, 2024 16:28

vytas7 added the blocked-by-release label Sep 20, 2024

bssyousefi and others added 3 commits September 23, 2024 13:29

fix linter error; remove unnecessary variable

ddfb904

Merge branch 'master' into Issue-475/add-docs-query-strings

8e02303

fix other linter errors

dfa33fd

CaselIT previously approved these changes Sep 23, 2024

View reviewed changes

examples/look/look/images.py Show resolved Hide resolved

examples/look/tests/test_app.py Outdated Show resolved Hide resolved

refactor: use dedicated attribute

b49d829

bssyousefi dismissed CaselIT’s stale review via b49d829 September 23, 2024 19:02

CaselIT approved these changes Sep 23, 2024

View reviewed changes

vytas7 removed the blocked-by-release label Oct 19, 2024

vytas7 and others added 2 commits October 19, 2024 19:22

Merge branch 'master' into Issue-475/add-docs-query-strings

92b2707

Merge branch 'master' into Issue-475/add-docs-query-strings

6484d52

vytas7 added 2 commits November 6, 2024 21:51

Merge branch 'master' into Issue-475/add-docs-query-strings

1271e78

Merge branch 'master' into Issue-475/add-docs-query-strings

7ae761e

vytas7 approved these changes Nov 7, 2024

View reviewed changes

vytas7 merged commit b765b25 into falconry:master Nov 7, 2024
31 checks passed

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

docs(user): add query strings tutorial #2239

docs(user): add query strings tutorial #2239

bssyousefi commented Jul 12, 2024 •

edited by vytas7

Loading

CaselIT left a comment

CaselIT Jul 13, 2024

CaselIT Jul 13, 2024

bssyousefi Jul 13, 2024

CaselIT Jul 13, 2024

CaselIT Jul 13, 2024

CaselIT Jul 13, 2024

CaselIT Jul 13, 2024

bssyousefi commented Jul 13, 2024

codecov bot commented Jul 18, 2024 •

edited

Loading

vytas7 left a comment

vytas7 Aug 25, 2024

bssyousefi Aug 26, 2024

bssyousefi Aug 26, 2024

vytas7 Aug 30, 2024

bssyousefi commented Aug 26, 2024

vytas7 Aug 30, 2024

bssyousefi commented Sep 23, 2024

vytas7 commented Sep 23, 2024

bssyousefi commented Sep 23, 2024

CaselIT left a comment

bssyousefi commented Nov 6, 2024

vytas7 commented Nov 6, 2024 •

edited

Loading

vytas7 left a comment

	{'href': '/images/'+image} for image in images
	{'href': '/images/' + image} for image in images

	max_size = int(req.params.get("maxsize", 0))
	max_size = int(req.get_param_as_int("maxsize", default=0, min_value=1))


		Let's go back to the ``on_get`` method and create a dynamic response. We can use query strings to set maximum image size and get the list of all images smaller than the specified value. We will use method ``get_param_as_int`` to set a default value of ``-1`` in case no ``maxsize`` query string was provided and also to enable a minimum value validation.

		.. code:: python

docs(user): add query strings tutorial #2239

docs(user): add query strings tutorial #2239

Conversation

bssyousefi commented Jul 12, 2024 • edited by vytas7 Loading

Summary of Changes

Related Issues

Pull Request Checklist

CaselIT left a 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

Choose a reason for hiding this comment

Choose a reason for hiding this comment

bssyousefi commented Jul 13, 2024

codecov bot commented Jul 18, 2024 • edited Loading

Codecov Report

vytas7 left a 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

bssyousefi commented Aug 26, 2024

Choose a reason for hiding this comment

bssyousefi commented Sep 23, 2024

vytas7 commented Sep 23, 2024

bssyousefi commented Sep 23, 2024

CaselIT left a comment

Choose a reason for hiding this comment

bssyousefi commented Nov 6, 2024

vytas7 commented Nov 6, 2024 • edited Loading

vytas7 left a comment

Choose a reason for hiding this comment

bssyousefi commented Jul 12, 2024 •

edited by vytas7

Loading

codecov bot commented Jul 18, 2024 •

edited

Loading

vytas7 commented Nov 6, 2024 •

edited

Loading