New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Update publishing sample code #394

Merged
merged 3 commits into from Jan 10, 2017

Conversation

Projects
None yet
4 participants
@sijis
Contributor

sijis commented Dec 13, 2016

This is an attempt to ensure the example code in the publishing section works.

See #304 for detail discussion on the issue.

@@ -49,10 +49,12 @@ See :doc:`development` on setting up your environment and workflow.
.. code-block:: python
import fedmsg
fedmsg.publish(topic='testing', modname='test', msg={
fedmsg.publish(name='mybus', topic='testing', modname='test', msg={

This comment has been minimized.

@pypingou

pypingou Dec 13, 2016

Member

The name is optional, isn't it?

This comment has been minimized.

@sijis

sijis Dec 13, 2016

Contributor

Yes it is, however, adding that parameter makes the example code easier to have work (and explain).
Otherwise, we have to explain that the default name is __main__.$(hostname -s), and that should be added as the entry in endpoints.py instead of something like mybus.

This comment has been minimized.

@pypingou

pypingou Dec 13, 2016

Member

Should we document both then?

This comment has been minimized.

@sijis

sijis Dec 13, 2016

Contributor

We could. It is a good idea actually.

So do you suggest I leave the original sample code and just note the endpoint.py entry based if the name parameter in publish() is used or not?

This comment has been minimized.

@pypingou

pypingou Dec 13, 2016

Member

Sounds good, or provide both example and explain how the endpoint.py should look in both cases

@sijis

This comment has been minimized.

Contributor

sijis commented Dec 14, 2016

@pypingou Added some additional changes. Let me know what you think.

.. code-block:: python
import fedmsg
fedmsg.publish(topic='testing', modname='test', msg={
'test': "Hello World",
})
.. note:: The ``endpoints.py`` file should have an entry as ``"__main__.myhost": [ "tcp://127.0.0.1:4321", ]``. The
value of ``__main__.myhost`` is autogenerated by default, and ``myhost`` is presented by the equivalent output

This comment has been minimized.

@pypingou

pypingou Dec 14, 2016

Member

Isn't __main__ the name of the program? (ie: on pkgdb, I'm seeing the endpoint pkgdb2.pkgdb01)?

So maybe we could put it explicitly, something like:

The endpoints.py file should have an entry as "<myprogram>.<myhost>": [ ...] where is the name of the program sending the message (can be __main__ if it is a simple script) and if the hostname of the machine sending the program (corresponds to the output of hostname -s).

This comment has been minimized.

@sijis

sijis Dec 14, 2016

Contributor

Updated the content.

@sijis

This comment has been minimized.

Contributor

sijis commented Dec 14, 2016

That's better said. I'll update the content accordingly.

@pypingou

This comment has been minimized.

Member

pypingou commented Dec 15, 2016

👍 for me, looking good! :)

@sijis

This comment has been minimized.

Contributor

sijis commented Dec 15, 2016

@glensc @odinho Do you think these doc changes clear up the confusion raised in issue #304?

@odinho

This comment has been minimized.

odinho commented Dec 16, 2016

@glensc

This comment has been minimized.

Contributor

glensc commented Dec 16, 2016

i will omit my answer in this case

@pypingou

This comment has been minimized.

Member

pypingou commented Jan 10, 2017

i will omit my answer in this case

I have no idea how to interpret this, do you not like the change and prefer to say nothing than saying something bad? You don't really care about the change so have nothing to say? You do like the change but you would prefer something else?

It would be really appreciated if you could extend a little on your opinion. This would help us improve the documentation or consider this as good to go and merge :)

Thanks in advance!

@glensc

This comment has been minimized.

Contributor

glensc commented Jan 10, 2017

merge it. keep up the good work. the bugreport is 2+ years old. and i don't use fedmsg, it takes too much effort for me to dig back in.

@pypingou

This comment has been minimized.

Member

pypingou commented Jan 10, 2017

Alright let's merge then :)

Thanks for your inputs everyone and thanks for the patch @sijis !

@pypingou pypingou merged commit 5ccc98b into fedora-infra:develop Jan 10, 2017

1 check passed

continuous-integration/travis-ci/pr The Travis CI build passed
Details

@sijis sijis deleted the sijis:doc/publish_messages branch Jan 10, 2017

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment