Skip to content

Conversation

@celinayk
Copy link
Contributor

What is this PR for?

This PR extracts hardcoded py4j values to constants in Python interpreters to improve maintainability. It creates a new PythonConstants class to centralize py4j version and file path constants, replacing hardcoded strings across multiple Python interpreter classes. This addresses the TODO comment requesting to avoid hardcoded py4j values.

What type of PR is it?

Refactoring

Todos

  • - Create PythonConstants class with py4j constants
  • - Replace hardcoded py4j values in IPythonInterpreter
  • - Replace hardcoded py4j values in PythonInterpreter
  • - Replace hardcoded py4j values in PythonDockerInterpreter

What is the Jira issue?

ZEPPELIN-6308

How should this be tested?

Screenshots (if appropriate)

Questions:

  • Does the license files need to update? No
  • Is there breaking changes for older versions? No
  • Does this needs documentation? No

Copy link
Contributor

@ParkGyeongTae ParkGyeongTae left a comment

Choose a reason for hiding this comment

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

It looks like the GitHub Action build failed due to line length errors. Could you please check and fix them?

Error: src/main/java/org/apache/zeppelin/python/PythonDockerInterpreter.java:[94] (sizes) LineLength: Line is longer than 100 characters (found 106). 
Error: src/main/java/org/apache/zeppelin/python/PythonInterpreter.java:[189] (sizes) LineLength: Line is longer than 100 characters (found 103). 
Error: src/main/java/org/apache/zeppelin/python/PythonInterpreter.java:[215] (sizes) LineLength: Line is longer than 100 characters (found 105).

@celinayk
Copy link
Contributor Author

It looks like the GitHub Action build failed due to line length errors. Could you please check and fix them?

Error: src/main/java/org/apache/zeppelin/python/PythonDockerInterpreter.java:[94] (sizes) LineLength: Line is longer than 100 characters (found 106). 
Error: src/main/java/org/apache/zeppelin/python/PythonInterpreter.java:[189] (sizes) LineLength: Line is longer than 100 characters (found 103). 
Error: src/main/java/org/apache/zeppelin/python/PythonInterpreter.java:[215] (sizes) LineLength: Line is longer than 100 characters (found 105).

Thanks! Fixed the line length issues by breaking the long lines to stay under 100 characters.

This was my first experience with automated code style checks - really appreciate how these guidelines help maintain code quality and readability across the project!

Copy link
Contributor

@ParkGyeongTae ParkGyeongTae left a comment

Choose a reason for hiding this comment

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

LGTM 👍

@ParkGyeongTae ParkGyeongTae self-assigned this Sep 7, 2025
@ParkGyeongTae ParkGyeongTae merged commit 4297661 into apache:master Sep 7, 2025
13 of 18 checks passed
ParkGyeongTae pushed a commit that referenced this pull request Sep 7, 2025
### What is this PR for?
This PR extracts hardcoded py4j values to constants in Python interpreters to improve maintainability. It creates a new PythonConstants class to centralize py4j version and file path constants, replacing hardcoded strings across multiple Python interpreter classes. This addresses the TODO comment requesting to avoid hardcoded py4j values.

### What type of PR is it?
Refactoring

### Todos
* [x] - Create PythonConstants class with py4j constants
* [x] - Replace hardcoded py4j values in IPythonInterpreter
* [x] - Replace hardcoded py4j values in PythonInterpreter
* [x] - Replace hardcoded py4j values in PythonDockerInterpreter

### What is the Jira issue?
[ZEPPELIN-6308](https://issues.apache.org/jira/browse/ZEPPELIN-6308)

### How should this be tested?

### Screenshots (if appropriate)

### Questions:
* Does the license files need to update? No
* Is there breaking changes for older versions? No
* Does this needs documentation? No

Closes #5056 from celinayk/ZEPPELIN-6308.

Signed-off-by: ParkGyeongTae <[email protected]>
(cherry picked from commit 4297661)
Signed-off-by: ParkGyeongTae <[email protected]>
@ParkGyeongTae
Copy link
Contributor

Merged into master and branch-0.12

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