Skip to content

[TE-4094] Add 'language_version' attribute to env var#67

Merged
gchan merged 2 commits intomainfrom
te-4094-add-a-integration-test-for-upload-example-which-caused-the
Jul 21, 2025
Merged

[TE-4094] Add 'language_version' attribute to env var#67
gchan merged 2 commits intomainfrom
te-4094-add-a-integration-test-for-upload-example-which-caused-the

Conversation

@gchan
Copy link
Contributor

@gchan gchan commented Jul 21, 2025

To collect Python language version usage and help us to better
plan deprecation support of end-of-life Python versions.

@gchan gchan force-pushed the te-4094-add-a-integration-test-for-upload-example-which-caused-the branch from 99ed77e to b79d0ad Compare July 21, 2025 01:27
@gchan gchan requested a review from a team July 21, 2025 01:28
"collector": f"python-{COLLECTOR_NAME}",
"version": VERSION
"version": VERSION,
"language_version": (
Copy link
Member

Choose a reason for hiding this comment

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

ChatGPT suggests this…

>>> import platform
>>> platform.python_version()
'3.13.2'

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Claude is saying the current implementation is better?

 Both sys.version_info and platform.python_version() are compatible with Python 3.9 and newer (and
  much older versions too). Here's the comparison:

  sys.version_info:
  - Returns a sys.version_info named tuple with attributes: major, minor, micro, releaselevel, serial
  - Direct access to version components
  - More efficient (no function call overhead)
  - Available since Python 2.0

  platform.python_version():
  - Returns a string in "major.minor.patchlevel" format
  - Requires importing platform module
  - Already formatted as a string
  - Available since Python 2.3

  Recommendation: Keep sys.version_info

  For your use case, sys.version_info is better because:

  1. More efficient - No function call or additional import needed
  2. More flexible - You can access individual components if needed later
  3. Lighter weight - sys is always imported, platform adds overhead
  4. More explicit - Shows exactly which version components you're using

  Your current implementation is optimal:
  f"{sys.version_info.major}.{sys.version_info.minor}.{sys.version_info.micro}"

  If you wanted to use platform.python_version(), you'd need:
  import platform
  # ...
  "language_version": platform.python_version()

Copy link
Member

Choose a reason for hiding this comment

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

My money is on platform.python_version() 😁

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 agree with ChatGPT and have updated the code to use 'system'. I've left the test as is to use platform so I'm not repeating the same expression in the test.

@gchan gchan force-pushed the te-4094-add-a-integration-test-for-upload-example-which-caused-the branch from b79d0ad to b8f4010 Compare July 21, 2025 02:52
gchan added 2 commits July 21, 2025 14:59
To collect Python language version usage and help us to better
plan deprecation support of end-of-life Python versions.
@gchan gchan force-pushed the te-4094-add-a-integration-test-for-upload-example-which-caused-the branch from b8f4010 to 92e8097 Compare July 21, 2025 03:01
@gchan
Copy link
Contributor Author

gchan commented Jul 21, 2025

The linter started to complain about a missing docstring for a new method recently added so I've added another commit to address that.

I have the incorrect branch name but I'm going to continue to roll with that and deal with Linear admin.

@gchan gchan requested a review from pda July 21, 2025 03:08
@gchan gchan merged commit 10625ca into main Jul 21, 2025
10 checks passed
@gchan gchan deleted the te-4094-add-a-integration-test-for-upload-example-which-caused-the branch July 21, 2025 03:15
@pda pda mentioned this pull request Jul 21, 2025
@nprizal nprizal changed the title [TE-4096] Add 'language_version' attribute to env var [TE-4094] Add 'language_version' attribute to env var Jul 22, 2025
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