Skip to content
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

Binding mqtt #9

Closed
wants to merge 17 commits into from
Closed

Binding mqtt #9

wants to merge 17 commits into from

Conversation

sebastiankb
Copy link
Contributor

Ready to merge.

@mkovatsc
Copy link
Contributor

Sorry, I cannot merge this:

  • The PR touches several files that have nothing to do with MQTT (e.g., HTTP binding, ContentSerdes, TD Tools!)
  • Commit messages do not conform with "conventional changelog"
  • Merges in between
  • Interactive rebase would need to redo almost all commits

Can you please:

  1. Clone the latest master
  2. Make a new MQTT Branch
  3. Copy in your binding-mqtt package
  4. Copy in your examples
  5. Do the required changes to cli and td-generator
  6. Open a new pull request

@sebastiankb
Copy link
Contributor Author

sebastiankb commented Jul 16, 2018

The PR touches several files that have nothing to do with MQTT (e.g., HTTP binding, ContentSerdes, TD Tools!)

All existing bindings need to be extended by the getAddress() method since this is requested by the protocol-interface.ts. During the TD generation this information is required to assign the correct broker address. Maybe you have an alternative idea?

ContentSerdes

Just a new line. Must be a mistake.

TD Tools

No changes there!?

Commit messages do not conform with "conventional changelog"

What do you mean?

Merges in between

That is the issue when a PR is not merged soon ;-)

Clone the latest master

This makes sense, however, please answer my question above before I do some changes.

@mkovatsc
Copy link
Contributor

The commit history even deletes complete files and puts them back later (e.g., d5ff2bf). This could be solved by squashing into one commit, but that is hindered/made too complex by the conflicts.

Commit messages do not conform with "conventional changelog"

https://conventionalcommits.org/ and https://wiki.siemens.com/display/en/Conventional+Changelog

The latter shows the concrete structural elements we are using (feat, fix, docs, style, ...). Since it is Siemens-internal, I will need to make a public version and link it from CONTRIBUTING.md.

We started using this quite some time ago already in the other repo.

Merges in between
That is the issue when a PR is not merged soon ;-)

It is much better when you rebase your branch on master whenever you need some updates from it, e.g., https://coderwall.com/p/9idt5g/keep-your-feature-branch-up-to-date (first best Google result..)

I understand all this is a bit more tedious, but it helps code quality and improves collaboration.

@sebastiankb
Copy link
Contributor Author

there is a new PR: #25

@sebastiankb sebastiankb deleted the binding-mqtt branch July 18, 2018 13:02
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.

None yet

2 participants