Add some cosmetics to identify the ReviewRequest ID #17
Conversation
There was a problem hiding this comment.
Hi Logu, Happy to see your contributions. I am tempted to say please add test coverage but I don't know if the surrounding code is already covered by automatic tests at all. Anyway, please take a look. Maybe it's not that hard :) and second there is a duplication which you can avoid by extracting a method
logsnaath
force-pushed
the
enhance_prompt
branch
2 times, most recently
from
5bc43dd to
43f928c
Compare
I dont know which duplication you have mentioned, is it leveraging subprocess instead of setproctitle ? |
mtui/main.py
Outdated
| @@ -45,6 +45,11 @@ def run_mtui(sys, config, logger, Prompt, Display, args): | |||
|
|
|||
| prompt = Prompt(config, logger, sys, Display) | |||
| if args.update: | |||
| short_rrid = ( | |||
| str(args.update.id).replace("SUSE", "S").replace("Maintenance", "M") | |||
mtui/prompt.py
Outdated
|
|
||
| def load_update(self, update, autoconnect): | ||
| tr = update.make_testreport(self.config, autoconnect=autoconnect) | ||
| self.metadata = tr | ||
| self.targets = tr.targets | ||
| self.set_prompt(None) | ||
| short_rrid = str(update.id).replace("SUSE", "S").replace("Maintenance", "M") |
logsnaath
force-pushed
the
enhance_prompt
branch
from
43f928c to
c072b3b
Compare
logsnaath
force-pushed
the
enhance_prompt
branch
from
c072b3b to
e2588e5
Compare
| .PHONY: tidy-diff | ||
| tidy-diff: | ||
| black --diff ./ | ||
|
|
There was a problem hiding this comment.
valid extension. But please make that a separate commit
| dboard_burl = "http://dashboard.qam.suse.de/incident/" | ||
| openqa_burl = "https://openqa.suse.de/tests/overview?build=:" |
There was a problem hiding this comment.
Please avoid specifying any SUSE internal URLs that are not relevant to this "upstream" open source project
There was a problem hiding this comment.
So should I avoid this change entirely?
or get the base urls from some other ways like the user environment?
or have them in mtui/config.py where a few simillar urls are already defined?
There was a problem hiding this comment.
reading from user environment or mtu/config.py both sounds good. I would just avoid specifying any SUSE internal URLs but for convenience maybe it makes sense to provide them but then please in a specific config file. https://github.com/openSUSE/mtui/blob/main/mtui/config.py#L114 looks similar so I guess it's ok if you define URLs there.
| @@ -443,6 +452,8 @@ def _show_yourself_data(self): | |||
| ("Build checks", self._testreport_url()[:-3] + "build_checks"), | |||
| ("Testreport", self._testreport_url()), | |||
| ("Repository", self.repository), | |||
| ("DashBoard Link", self._get_result_links()[0]), | |||
| ("OpenQA Link", self._get_result_links()[1]), | |||
There was a problem hiding this comment.
| ("OpenQA Link", self._get_result_links()[1]), | |
| ("openQA Link", self._get_result_links()[1]), |
There was a problem hiding this comment.
I would also lowercase "Link" here, as well as in the line above. If this ends up rendered as link then maybe also drop " Link" completely. The other entries also don't have " Link".
| @@ -443,6 +452,8 @@ def _show_yourself_data(self): | |||
| ("Build checks", self._testreport_url()[:-3] + "build_checks"), | |||
| ("Testreport", self._testreport_url()), | |||
| ("Repository", self.repository), | |||
| ("DashBoard Link", self._get_result_links()[0]), | |||
There was a problem hiding this comment.
| ("DashBoard Link", self._get_result_links()[0]), | |
| ("Dashboard Link", self._get_result_links()[0]), |
| @@ -1,6 +1,7 @@ | |||
| import logging | |||
| import sys | |||
| from subprocess import CalledProcessError | |||
| import setproctitle | |||
| @@ -11,6 +12,7 @@ | |||
|
|
|||
| from .argparse import ArgsParseFailure | |||
| from .colorlog import create_logger | |||
| from .utils import get_short_rrid | |||
There was a problem hiding this comment.
why special function if this can be method of RequestReviewID class ?
| @@ -45,6 +46,9 @@ def run_mtui(sys, config, logger, Prompt, Display, args): | |||
|
|
|||
| prompt = Prompt(config, logger, sys, Display) | |||
| if args.update: | |||
| short_rrid = get_short_rrid(str(args.update.id)) | |||
| # helps to set window/tab title with reviewid | |||
| setproctitle.setproctitle("mtui-" + short_rrid) | |||
There was a problem hiding this comment.
btw how will load_template command change title ?
| dboard_burl = "http://dashboard.qam.suse.de/incident/" | ||
| openqa_burl = "https://openqa.suse.de/tests/overview?build=:" | ||
| dboard_lnk = dboard_burl + incident_id | ||
| openqa_lnk = openqa_burl + incident_id + ":" + self.srpms[0] |
|
|
||
| self._attrs = [ | ||
| "products", | ||
| "category", | ||
| "packager", | ||
| "reviewer", | ||
| "packages", | ||
| "srpms", |
There was a problem hiding this comment.
and parser for srpms is where ? and data source ?
| @@ -333,3 +333,7 @@ def walk(inc): | |||
| if isinstance(inc[key], (list, dict)): | |||
| inc[key] = walk(inc[key]) | |||
| return inc | |||
|
|
|||
|
|
|||
| def get_short_rrid(id_str): | |||
There was a problem hiding this comment.
why ? this should be method of RequestReviewID ...
S:M:xxxxx:yyyyyy/mtui-auto>. This requirespython311-settproctitle.Would like to get more insights and comments.