Duplicate Flags in Makefile #11662

vignesh1507 · 2024-10-09T18:39:19Z

Duplicate Flags: In the upgrade-pinned-dependencies target, --strip-extras is specified twice in the pip-compile commands:

$(VENV_RUN); pip-compile --strip-extras --upgrade --strip-extras -o requirements-basic.txt pyproject.toml

changes made to the code are as follows: 1. The regular expression r"\smypy_boto3_([a-z0-9_]+)\s" looks for occurrences of mypy_boto3_ followed by alphanumeric characters and underscores. This pattern assumes that there will always be a space before and after the match. However, if mypy_boto3_ appears at the beginning of a line or is surrounded by something other than spaces (like parentheses, quotes, etc.), it won't match. A slightly more robust regex could be: r"mypy_boto3_([a-z0-9_]+)", which would match mypy_boto3_ patterns regardless of their surrounding whitespace. 2. No Output If No Match: If result is empty (i.e., no match is found), the script will still print pip install "boto3-stubs[]". It would be better to check if result contains items before printing the pip install command.

Duplicate Flags: In the upgrade-pinned-dependencies target, --strip-extras is specified twice in the pip-compile commands: ```bash $(VENV_RUN); pip-compile --strip-extras --upgrade --strip-extras -o requirements-basic.txt pyproject.toml ```

bentsku

Hello and thanks for your contribution! Are you using an automated tool for grammar? It seems it was a bit too eager and changed things that do not make sense anymore. Could you revert the commented changes?

I'll ping @silv-io for the change in the Makefile in particular!

edit: I realize some of these changes have been already merged, as your commits are 3 weeks old. There also are some merge conflicts. Could you please rebase your PR?

bentsku · 2024-10-10T10:13:19Z

localstack-core/localstack/config.py

@@ -338,7 +338,7 @@ def in_docker():
        if os_hostname and os_hostname in content:
            return True

-    # containerd does not set any specific file or config, but does use
+    # container does not set any specific file or config, but it does use


This is a wrong automated change, we are indeed talking about containerd

bentsku · 2024-10-10T10:13:56Z

localstack-core/localstack/config.py

@@ -375,7 +375,7 @@ def in_docker():
    return False


-# whether the in_docker check should always return True or False
+# whether in_docker check should always return True or False


I believe the readability is worse with this change

bentsku · 2024-10-10T10:14:47Z

localstack-core/localstack/config.py

-# will resolve to the real DNS entry, rather than the local one.
+# will resolve it to the real DNS entry, rather than the local one.


this is a wrong change, the sentence is "Any DNS name not matched * will resolve to the... ", the it does not make sense

bentsku · 2024-10-10T10:15:28Z

localstack-core/localstack/config.py

-    internal use. This function is also not needed anymore because we don't separate between HTTP
-    and HTTP ports anymore since LocalStack listens to both."""
+    internal use. This function is not needed anymore because we don't separate between HTTP
+    and HTTP ports anymore since LocalStack comprehends to both."""


this change does not make sense, this talks about listening to a port, and this does not mean anything?

bentsku · 2024-10-10T10:15:53Z

localstack-core/localstack/constants.py

@@ -33,7 +33,7 @@
 # Artifacts endpoint
 ASSETS_ENDPOINT = "https://assets.localstack.cloud"

-# host to bind to when starting the services
+# host to bind when starting the services


this is wrong, we "bind to" a host and not bind a host

bentsku · 2024-10-10T10:16:37Z

localstack-core/localstack/deprecations.py

@@ -327,7 +327,7 @@ def deprecated_endpoint(
    endpoint: Callable, previous_path: str, deprecation_version: str, new_path: str
 ) -> Callable:
    """
-    Wrapper function which logs a warning (and a deprecation path) whenever a deprecated URL is invoked by a router.
+    Wrapper function which logs a warning (and a deprecation path) whenever a deprecated URL is invoked by the router.


we can have different routers, so a is more correct

bentsku · 2024-10-10T10:18:07Z

scripts/generate_minimal_boto3stubs_install.py


-        print(f'pip install "boto3-stubs[{",".join(result)}]"', end="")
+    if os.path.exists(filepath):


why is this change needed? could you explain it?

bentsku · 2024-10-10T10:18:35Z

localstack-core/localstack/config.py

@@ -32,8 +32,8 @@

 class Directories:
    """
-    Holds the different directories available to localstack. Some directories are shared between the host and the
-    localstack container, some live only on the host and some only in the container.
+    Holds different directories available to localstack. Some directories are shared between the host and the


remove the the makes it a bit less readable in my opinion

silv-io

The change in the Make target looks good!

Before merging, I would just ask to remove all the other changes (seems like they were automated?) as they don't seem to be connected to the PR description and goal.

vignesh1507 · 2024-10-10T10:37:52Z

ok then shall i delete the files except the makefile one?

silv-io · 2024-10-10T10:39:06Z

@vignesh1507 yes, please just remove the changes outside of the Makefile :)

silv-io · 2024-10-10T11:23:26Z

@vignesh1507 I think there was a bit of a misunderstanding here. We would ask you to revert all the changes outside of the Makefile, not delete those files. To do that, you can do e.g.:

git rebase -i master

And then you can replace all occurrences of pick with drop in all commits except "Update Makefile"

After that you can do git push --force and the changeset should only include the change for the Makefile

vignesh1507 · 2024-10-10T12:45:59Z

Made a new PR (#11666 ) with only the makefile commit.

vignesh1507 and others added 8 commits September 20, 2024 13:28

Update config.py

aaff59b

Update constants.py

6979fe8

Update deprecations.py

b020354

Update build_common_test_functions.sh

bd70cea

Update gather_outdated_snapshots.py

8363c01

Update capture_notimplemented_responses.py

1671ca1

Update Makefile

a5a0885

Duplicate Flags: In the upgrade-pinned-dependencies target, --strip-extras is specified twice in the pip-compile commands: ```bash $(VENV_RUN); pip-compile --strip-extras --upgrade --strip-extras -o requirements-basic.txt pyproject.toml ```

vignesh1507 requested a review from thrau as a code owner October 9, 2024 18:39

bentsku requested changes Oct 10, 2024

View reviewed changes

bentsku requested a review from silv-io October 10, 2024 10:20

silv-io requested changes Oct 10, 2024

View reviewed changes

vignesh1507 added 6 commits October 10, 2024 16:10

Delete localstack-core/localstack/config.py

d3353b3

Update constants.py

0bb896b

Delete localstack-core/localstack/constants.py

54cb981

Delete localstack-core/localstack/deprecations.py

7b0efb1

Delete scripts/build_common_test_functions.sh

e526ece

Delete scripts/capture_notimplemented_responses.py

bee3952

silv-io added the semver: patch Non-breaking changes which can be included in patch releases label Oct 10, 2024

vignesh1507 closed this Oct 10, 2024

vignesh1507 deleted the version-0.1 branch October 10, 2024 12:30

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

Uh oh!

Uh oh!

Duplicate Flags in Makefile #11662

Duplicate Flags in Makefile #11662

Uh oh!

vignesh1507 commented Oct 9, 2024

Uh oh!

bentsku left a comment •

edited

Loading

Uh oh!

bentsku Oct 10, 2024

Uh oh!

bentsku Oct 10, 2024

Uh oh!

bentsku Oct 10, 2024

Uh oh!

bentsku Oct 10, 2024

Uh oh!

bentsku Oct 10, 2024

Uh oh!

bentsku Oct 10, 2024

Uh oh!

bentsku Oct 10, 2024

Uh oh!

bentsku Oct 10, 2024

Uh oh!

silv-io left a comment

Uh oh!

vignesh1507 commented Oct 10, 2024

Uh oh!

silv-io commented Oct 10, 2024

Uh oh!

silv-io commented Oct 10, 2024

Uh oh!

vignesh1507 commented Oct 10, 2024

Uh oh!

Uh oh!

		# will resolve to the real DNS entry, rather than the local one.
		# will resolve it to the real DNS entry, rather than the local one.


		print(f'pip install "boto3-stubs[{",".join(result)}]"', end="")
		if os.path.exists(filepath):

Uh oh!

Duplicate Flags in Makefile #11662

Duplicate Flags in Makefile #11662

Uh oh!

Conversation

vignesh1507 commented Oct 9, 2024

Uh oh!

bentsku left a comment • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

silv-io left a comment

Choose a reason for hiding this comment

Uh oh!

vignesh1507 commented Oct 10, 2024

Uh oh!

silv-io commented Oct 10, 2024

Uh oh!

silv-io commented Oct 10, 2024

Uh oh!

vignesh1507 commented Oct 10, 2024

Uh oh!

Uh oh!

bentsku left a comment •

edited

Loading