Bug: Port is ignored when specified from extra_args #84
Conversation
Mainframe3270/x3270.py
Outdated
| if posport != -1: | ||
| argport = resval[posport+6::].strip() | ||
| if argport != "": | ||
| self.port = argport |
There was a problem hiding this comment.
I do not like this implementation. As I suggested in my comment in #80 , if the -port option is specified in extra_args this should be used, but the user should receive some type of hint
logger.warn(
"The port was specified both from the `port` as well as from the `extra_args` parameter."
"The one specified in extra_args will be used."
)
I will investigate a way of doing this more elegantly. But this should not prevent you from finding one as well
There was a problem hiding this comment.
I agree but how can I discern between a specified port= or set by default through port: int = 23,?
There was a problem hiding this comment.
I made some comments to your code.
Generally, I like the thoroughness of this PR. You even went and changed the faulty x3270 command line options in the utests!
However, some additional work is still required:
- Firstly
inv lintcaused a failure of the pipeline. Please run this locally and review the errors - You can remove the
ExecutableAppclass and its occurrences in py3270. Let me know if you need help with this. - We should find a better way of using the port if it was provided in extra_args, and then give a notification that the port was used from the command line options.
|
@robinmatz You did not respond to my question on the inv lint |
|
@Harm10 , I created a PR on your fork with my suggested changes. Please see my comments there. |
Codecov ReportPatch coverage:
📣 This organization is not using Codecov’s GitHub App Integration. We recommend you install it so Codecov can continue to function properly for your repositories. Learn more Additional details and impacted files@@ Coverage Diff @@
## master #84 +/- ##
==========================================
+ Coverage 93.45% 94.32% +0.86%
==========================================
Files 4 4
Lines 581 581
Branches 88 90 +2
==========================================
+ Hits 543 548 +5
+ Misses 32 28 -4
+ Partials 6 5 -1
Flags with carried forward coverage won't be shown. Click here to find out more.
Help us with your feedback. Take ten seconds to tell us how you rate us. Have a feature suggestion? Share it here. ☔ View full report in Codecov by Sentry. |
|
I merged your PR. Please see my comments there. |
|
Thanks. There are some parts of the code that I want to optimize still. Also I will write a few more tests to increase the statement coverage. |
|
@Harm10 I started a draft PR on your repo where I will do the remaining changes |
|
@Harm10 - I finished the changes on my PR on your fork. Please review them at any time |
Open connection port in args rework
|
@robinmatz Whatever I try the connection is no longer build to pub400. Even when only specifying the host there is no connection. Will retry later. |
|
@robinmatz I retried with my updated master to also not get any connection. So I guess this is due to PUB400? |
robinmatz
changed the title
|
@robinmatz I sent you a remark on RF slack. Are you active on that platform? I want to know where the logger.warn message should be visible. |
@robinmatz This is my change for Open connection. To check whether the extra_args contain a port option you need to process them before the Emulator is created. So I copied your updated function from py3270.py and put it logically (it has to return something) in x3270.py.
For this I have several questions and remarks:
argport = resval[posport+6::].strip()toargport = resval[posport + 6 : : ].strip()and I get a messageMainframe3270/x3270.py:102:53: E203 whitespace before ':'twice. How to circumvent this? I left the code in the original form.Any other suggestions?