pytroll / pytroll-collectors

Collector modules for Pytroll
GNU General Public License v3.0
3 stars 18 forks source link

Change publish topic to start with the instrument name #125

Closed adybbroe closed 1 year ago

adybbroe commented 1 year ago

This PR introduce the instrument name to the publish topic so e.g. that instead of /RDR/0... it would be /atms/RDR/0...

codecov[bot] commented 1 year ago

Codecov Report

Merging #125 (4ec4c24) into main (e3b9abc) will increase coverage by 0.31%. The diff coverage is 98.14%.

@@            Coverage Diff             @@
##             main     #125      +/-   ##
==========================================
+ Coverage   91.32%   91.64%   +0.31%     
==========================================
  Files          27       27              
  Lines        4044     4115      +71     
==========================================
+ Hits         3693     3771      +78     
+ Misses        351      344       -7     
Flag Coverage Δ
unittests 91.64% <98.14%> (+0.31%) :arrow_up:

Flags with carried forward coverage won't be shown. Click here to find out more.

Impacted Files Coverage Δ
pytroll_collectors/scisys.py 63.80% <80.00%> (+1.61%) :arrow_up:
pytroll_collectors/tests/test_scisys.py 99.28% <99.23%> (+4.49%) :arrow_up:
pytroll_collectors/fsspec_to_message.py 98.03% <100.00%> (+0.10%) :arrow_up:
pytroll_collectors/s3stalker.py 100.00% <100.00%> (ø)
pytroll_collectors/tests/test_fsspec_to_message.py 93.71% <100.00%> (+0.42%) :arrow_up:
pytroll_collectors/tests/test_s3stalker.py 99.47% <100.00%> (+0.01%) :arrow_up:
adybbroe commented 1 year ago

One unhandled comment in the tests.

And how about the user-configurable topic?

Sorry, what do you mean "And how about the user-configurable topic"?

And concerning testing the private interface, what is the issue? What do you suggest? I re factored out an untested part in receive_from_zmq to test it, but I think it should still be a private function. Testing the entire receive_from_zmq as is, is beyond what I am capable of.

pnuu commented 1 year ago

One unhandled comment in the tests. And how about the user-configurable topic?

Sorry, what do you mean "And how about the user-configurable topic"?

I can't find my previous comment, but should it be possible for the user to configure the topic instead of forcing the topic to be what scisys receiver is now setting it to?

And concerning testing the private interface, what is the issue?

Private interface indicates that it's an implementation detail and might change.

What do you suggest?

What we normally do is to create a test that calls the public interface and check that it produces the expected result with different settings.

adybbroe commented 1 year ago

I have converted the private function to a public one. I suggest stopping the re-factoring here. This part of the project is in need for some more care and refactoring and increased test coverage, but I really don't think this belongs here. I hope you can both agree @mraspaud and @pnuu ?

coveralls commented 1 year ago

Pull Request Test Coverage Report for Build 4809005285


Totals Coverage Status
Change from base Build 4806652631: 0.0%
Covered Lines: 0
Relevant Lines: 0

💛 - Coveralls
adybbroe commented 1 year ago

Thanks @pnuu Yes, we are likely the only ones. And we will probably not have this CGI based system in a year from now, and as you said this code block was not tested before either. So anyhow this is an incremental improvement. Maybe just let @mraspaud see if he agress before merging?