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

Adapt to core changes (addon.xml) #1872

Merged
merged 2 commits into from
Jan 16, 2023
Merged

Adapt to core changes (addon.xml) #1872

merged 2 commits into from
Jan 16, 2023

Conversation

J-N-K
Copy link
Member

@J-N-K J-N-K commented Aug 19, 2022

Depends on openhab/openhab-core#3050

Signed-off-by: Jan N. Klug [email protected]

@netlify
Copy link

netlify bot commented Aug 19, 2022

Thanks for your pull request to the openHAB documentation! The result can be previewed at the URL below (this comment and the preview will be updated if you add more commits).

Name Link
🔨 Latest commit e68d455
🔍 Latest deploy log https://app.netlify.com/sites/openhab-docs-preview/deploys/63c5a66fce13d1000986de7c
😎 Deploy Preview https://deploy-preview-1872--openhab-docs-preview.netlify.app
📱 Preview on mobile
Toggle QR Code...

QR Code

Use your smartphone camera to open QR code link.

To edit notification comments on pull requests, go to your Netlify site settings.

@Confectrician
Copy link
Contributor

Confectrician commented Aug 21, 2022

1800 changed lines are looking wrong.

Can you check if something went wrong with the commits on your side?
Looks like some of the changed files have been removed and readded identically oer line.

@J-N-K
Copy link
Member Author

J-N-K commented Aug 21, 2022

Looks like line-ending changes. According to https://github.com/openhab/openhab-docs/blob/main/.editorconfig CRLF is expected, what is exactly what I committed. Can you comment what is the correct line-ending (it seems it was LF before, according to my local git client).

@Confectrician
Copy link
Contributor

It is ok for me the change the line endings to crlf.
I have no strong opinion on this and the file hasn't been changed in any valuable part of the suggestions.

Maybe it isn't used that often and we missed those files for a long time.
If you are fine with having that much changes in here, that would be ok for me as it is for a reason.

But ofc those changes are a bit misleading to the original intention and pr title.

@J-N-K
Copy link
Member Author

J-N-K commented Aug 23, 2022

I can revert that, not an issue. It wasn‘t even intentional, my IDE just picked up what the repo provided as suggestion. I believe in most other repos we use only LF.

@Confectrician
Copy link
Contributor

Then i should check this in egenral and adapt the config to the generic approach, where LF would be fine at all.

In this case reverting would be good for th epr to just reflect the intentional changes.

@Confectrician Confectrician added the Feedback Waiting for feedback label Sep 10, 2022
@Confectrician Confectrician added this to the 3.4 milestone Sep 10, 2022
Confectrician
Confectrician previously approved these changes Nov 18, 2022
Copy link
Contributor

@Confectrician Confectrician left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Sorry, this went out of my focus for way too long.
I didn't realise, that you already fixed everything with the commits.

I have just one small finding, which i will solve myself.

@Confectrician Confectrician dismissed their stale review November 18, 2022 23:10

Found needed changes after solving the merge conflicts.

Copy link
Contributor

@Confectrician Confectrician left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Again some small comments/questions, from checking the preview after solving the conflicts.

@Confectrician Confectrician removed the Feedback Waiting for feedback label Nov 18, 2022
@J-N-K
Copy link
Member Author

J-N-K commented Nov 19, 2022

Please make sure that this is only merged after the core-PR has been merged, otherwise we'll have an inconsistent documentation.

@Confectrician Confectrician added the stat: dependency 💥 This issue/pr has a dependency in another repo label Nov 19, 2022
@Confectrician
Copy link
Contributor

I have added the dependency label and subscribed to the core pr. :)

@Confectrician Confectrician modified the milestones: 3.4, 3.5 Dec 19, 2022
@@ -142,7 +142,7 @@ module.exports = [
children: [
['developer/', 'Overview & Introduction'],
'developer/guidelines',
'developer/bindings/',
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I would definitely suggest to keep the "bindings" menu in place, just as we have dedicated entries for other add-on types like automation addons, transformation services, voice add-ons etc.
What would imho make sense here is to add a new menu entry for everything that generally applies to all add-ons and this new page could then link to the different specific add-on pages as well.

Copy link
Member Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

The problem is that we don't have content for that page. Duplicating what is written on the general page makes no sense.

Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Well, a few parts could be moved from the binding page to the add-on page, like the description about the addon.xml, the info on how to add a logo for the add-on and similar.

Copy link
Member Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

After I did that, there was no content left :-)

Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Hm, I see your point, but I am a bit puzzled, because on https://www.openhab.org/docs/developer/bindings/ I see a lot more content about how to develop a binding. You seem to have moved all the files that contribute to this page to /developer/addon.

Copy link
Member Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Sometimes I'm a bit slow... Is this version better?

Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Yes, thanks - that look better!

@J-N-K J-N-K force-pushed the addons-xml branch 2 times, most recently from 6465800 to 5b2e346 Compare January 16, 2023 18:17
Copy link
Member

@kaikreuzer kaikreuzer left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Do you have any idea, why Github does not properly show the changes that you made, but rather shows the full files to be changed (e.g. developers/bindings/index.md)?
This makes reviewing hard, but I just trust that there were no irrelevant changes.

title: Add-ons
---

# Developing an add-on
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Suggested change
# Developing an add-on
# Developing an Add-on

Copy link
Member Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Yes, that's due to an incorrect .editorconfig in this repo (it defines CRLF instead of LF which is actually used).

Copy link
Member Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

This should be fine now.

@@ -142,7 +142,7 @@ module.exports = [
children: [
['developer/', 'Overview & Introduction'],
'developer/guidelines',
'developer/bindings/',
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Yes, thanks - that look better!

Signed-off-by: Jan N. Klug <[email protected]>
Signed-off-by: Jan N. Klug <[email protected]>
Copy link
Member

@kaikreuzer kaikreuzer left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Wonderful - that's how it should look like! 👍

wborn added a commit to wborn/openhab-docs that referenced this pull request Jul 29, 2023
Some documentation files were moved in openhab#1872 which caused these links to be broken.

Signed-off-by: Wouter Born <[email protected]>
@wborn wborn mentioned this pull request Jul 29, 2023
Confectrician pushed a commit that referenced this pull request Jul 30, 2023
Some documentation files were moved in #1872 which caused these links to be broken.

Signed-off-by: Wouter Born <[email protected]>
@github-actions github-actions bot mentioned this pull request Jul 30, 2023
github-actions bot pushed a commit that referenced this pull request Jul 30, 2023
Some documentation files were moved in #1872 which caused these links to be broken.

Signed-off-by: Wouter Born <[email protected]>
stefan-hoehn pushed a commit that referenced this pull request Nov 19, 2023
Some documentation files were moved in #1872 which caused these links to be broken.

Signed-off-by: Wouter Born <[email protected]>
Co-authored-by: Wouter Born <[email protected]>
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
stat: dependency 💥 This issue/pr has a dependency in another repo
Projects
None yet
Development

Successfully merging this pull request may close these issues.

3 participants