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.

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

feat: Support multiple extras in requirements constraint advertisement #379

Merged
merged 6 commits into from
Aug 16, 2023

Conversation

timmc-edx
Copy link
Contributor

@timmc-edx timmc-edx commented Aug 9, 2023

Update the base template's setup.py to support the full syntax of package extras.

  • Support commas and whitespace inside extras declarations, e.g. some_package[extra_one, extra_two]
  • Raise exception if we spell packages differently in requirement and constraint files, including presence/absence of extras.

Merge checklist:
Check off if complete or not applicable:

  • Changelog record added
  • Documentation updated (not only docstrings)
  • Fixup commits are squashed away
  • Unit tests added/updated
  • Manual testing instructions provided
  • Noted any: Concerns, dependencies, deadlines, tickets

@timmc-edx timmc-edx marked this pull request as draft August 9, 2023 21:22
Update the base template's setup.py to support the full syntax of package
extras.

- Support commas and whitespace inside extras declarations, e.g.
  `some_package[extra_one, extra_two]`
- Raise exception if we spell packages differently in requirement and
  constraint files, including presence/absence of extras.
@timmc-edx
Copy link
Contributor Author

I should figure out how to do automated testing of this. For now, some manual testing:

  1. In event-bus-kafka, change setup.py to read from test.in rather than base.in (so that there's a package with extras in the requires)
  2. From event-bus-kafka repo, run the cookiecutters update_setup_py.sh script. Read .git/cleanup-python-code-description and observe that confluent_kafka was without its extras in the old requires, but carries the extras in the new requires.
  3. Add confluent_kafka<=2.3.0 to the constraints.txt file. Run python setup.py bdist_wheel and observe the error:

    Exception: Encountered both "confluent_kafka[avro,schema-registry]" and "confluent_kafka" in requirements and constraints files; please use just one or the other.

f'Encountered both "{seen_spelling}" and "{package}" in requirements '
'and constraints files; please use just one or the other.'
)

requirements = {}
constraint_files = set()

# groups "pkg<=x.y.z,..." into ("pkg", "<=x.y.z,...")
Copy link
Contributor

Choose a reason for hiding this comment

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

can we update the comment to include how the regex treats [] ? it's a pretty hard regex to read

Copy link
Contributor Author

Choose a reason for hiding this comment

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

How would you feel about using verbose mode? Not something I think I've ever used before, but I agree that this regex is... pretty dense.

    requirement_line_regex = re.compile(
        r"""
          (
            # The base name of the package
            [%s]+
            # Optionally, an extras section, as in `some-package[extra-one, extra-two]`
            (?:
              \[[%s,\s]+\]
            )?
          )

          # Optionally, a version constraint like `>=3.0`
          (
            [<>=][^#\s]+
          )?
        """
        % (re_package_name_base_chars, re_package_name_base_chars),
        re.VERBOSE
    )

Or even using a raw format-string, which is apparently a thing you can do? And maybe adding some spaces inside some of the more confusing character classes:

    requirement_line_regex = re.compile(
        fr"""
          (
            # The base name of the package
            [ {re_package_name_base_chars} ]+
            # Optionally, an extras section, as in `some-package[extra-one, extra-two]`
            (?:
              \[ [{re_package_name_base_chars},\s]+ \]
            )?
          )

          # Optionally, a version constraint like `>=3.0`
          (
            [<>=][^#\s]+
          )?
        """,
        re.VERBOSE
    )

Copy link
Contributor Author

Choose a reason for hiding this comment

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

(I've pushed up a far simpler option. Verbose mode is cool but... probably overkill here.)

@@ -36,17 +36,43 @@ def load_requirements(*requirements_paths):
with -c in the requirements files.
Returns a list of requirement strings.
"""
by_canonical_name = {}
Copy link
Contributor

Choose a reason for hiding this comment

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

A comment here with an example entry would be helpful. It took me a minute to figure out how it was being used.

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Sure, maybe something like this:

    # e.g. {"django": "Django", "openedx-events": "openedx_events"}

CHANGELOG.rst Outdated
=======

- In setup.py, support advertising constraints on packages with multiple extras
- Fail packaging if requirements are spelled differently in different places
Copy link
Contributor

Choose a reason for hiding this comment

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

Suggested change
- Fail packaging if requirements are spelled differently in different places
- Fail packaging if requirements are named differently in different places or have different extras added

"""
Raise exception if package is spelled different ways.

This ensures that packages are spelled consistently so we can match
Copy link
Contributor

Choose a reason for hiding this comment

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

It's not really spelling.

Suggested change
This ensures that packages are spelled consistently so we can match
This ensures that packages are named consistently so we can match

Copy link
Contributor

@rgraber rgraber left a comment

Choose a reason for hiding this comment

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

One suggestion for comment, otherwise LGTM

@@ -36,17 +36,45 @@ def load_requirements(*requirements_paths):
with -c in the requirements files.
Returns a list of requirement strings.
"""
# e.g. {"django": "Django", "openedx-events": "openedx_events"}
Copy link
Contributor

Choose a reason for hiding this comment

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

Just to be super de dooper clear

Suggested change
# e.g. {"django": "Django", "openedx-events": "openedx_events"}
# e.g. {"django": "Django", "openedx-events": "openedx_events", "confluent-kafka":"confluent-kafka[avro]"}

Copy link
Contributor Author

Choose a reason for hiding this comment

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

The quality checks will probably yell about line length. Maybe I'll use confluent_kafka[avro] to demonstrate both underscore and extras?

@timmc-edx timmc-edx merged commit f6e6a2d into master Aug 16, 2023
4 checks passed
@timmc-edx timmc-edx deleted the timmc/package-regex branch August 16, 2023 20:52
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

2 participants