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

Renamed namespace "org.eclipse.smarthome" to "org.openhab.core" #1294

Merged
merged 3 commits into from
Dec 28, 2019

Conversation

cweitkamp
Copy link
Contributor

@cweitkamp cweitkamp commented Dec 23, 2019

  • Renamed namespace "org.eclipse.smarthome" to "org.openhab.core"

Closes #1290

No changes in files except renaming of namespace and QualifiedName definitions in bundles/org.openhab.core.model.script/src/org/openhab/core/model/script/scoping/ScriptImportSectionNamespaceScopeProvider.java.

One disabled integration test: org.openhab.core.model.script.tests.

Signed-off-by: Christoph Weitkamp github@christophweitkamp.de

@Hilbrand
Copy link
Member

What version of the script did you use? I was working on it too and had the same tests fail. However I first had a problem with the sed replace (earlier version had 3, now 4 sed calls) The earlier version missed namespace in the xtend among other things. I've also added a specific patch for quartz-test.properties also in same problematic test.

I was looking at why it didn't run the tests. It doesn't seem to be able to find DecimalType and QuantityType classes in rules.

@cweitkamp
Copy link
Contributor Author

I think I used revision 2 of your script.

Yes, I saw an error related to QuantityType too.

@Hilbrand
Copy link
Member

Hilbrand commented Dec 23, 2019

I clicked your link to scriptimport but it took some time to reload, so I missed that while typing my message. But that maybe what I was missing. I've updated your changes locally and it's now running again. Also I use a separate clean local maven repo as it conflicts with current other builds.

@cweitkamp
Copy link
Contributor Author

cweitkamp commented Dec 23, 2019

I submitted my WIP branch for IDE openhab/openhab-distro#1030.

There seem to be a problem with discovery service(s). They do not work. OSGi is not able to initialize the components when running from Eclipse IDE. The other core bundles seem to work - without further testing.

@cweitkamp
Copy link
Contributor Author

cweitkamp commented Dec 23, 2019

Integration tests for org.openhab.core.model.script.tests are working now.

@Hilbrand
Copy link
Member

image
With version 5 of my script.

@cweitkamp
Copy link
Contributor Author

👍

One question regarding the naming:

We now renamed a lot of packages from org.eclipse.smarthome.* to org.openhab.* (e.g. org.eclipse.smarthome.config.discovery.internal.console to org.openhab.config.discovery.internal.console). But the Maven artifact is org.openhab.core.config.discovery. Shouldn't we move / rename the packages to org.openhab.core.* (e.g. org.openhab.core.config.discovery.internal.console) instead?

@kaikreuzer
Copy link
Member

Shouldn't we move / rename the packages to org.openhab.core.*

Yes, absolutely! Just as I said here.

@Hilbrand
Copy link
Member

That does seem to make sense. I does require some more work because some classes are already in a core package and simply adding core to my script causes them to go into core.core. Also it is theoretical possible that classes end up in the same namespace if one is moved to core and the other was already in core. But we'll need to analyze that.

@kaikreuzer
Copy link
Member

I guess the only exception has to be made for the https://github.com/openhab/openhab-core/tree/master/bundles/org.openhab.core bundle.
Afaik, there should be no clashes - if there are, we probably have a bad API, which would need to be fixed anyhow.

@Hilbrand
Copy link
Member

I've updated the script it now puts all in core. I've pushed a branch with the changes here: https://github.com/Hilbrand/openhab-core/tree/1290/
It still compiles with tests. But I haven't run it yet.

@cweitkamp
Copy link
Contributor Author

Do you like to commit your latest changes against my branch of should we replace this one.

@Hilbrand
Copy link
Member

You can simple git reset --hard your branch against master and rerun the script. Would be a good test if the script not only works on my machine.

@cweitkamp
Copy link
Contributor Author

cweitkamp commented Dec 26, 2019

I applied your script again. The replacement part in corePatches did not work. After changing it manually I got a green build.

// EDIT: I think your script requires a path as parameter $1. I did not provide this parameter but running it from my openhab-core/ folder directly.

@cweitkamp cweitkamp changed the title Renamed namespace "org.eclipse.smarthome" to "org.openhab" Renamed namespace "org.eclipse.smarthome" to "org.openhab.core" Dec 26, 2019
@cweitkamp cweitkamp reopened this Dec 26, 2019
@kaikreuzer
Copy link
Member

One observation: This PR now has all files deleted and new ones created - which means that we will lose the complete history of the changes on the files (ok, not technically, but practically as something like "git blame" won't work anymore).

Would it be possible to enhance the script to do a 2-step approach? First moving the files to the new location (while keeping their content untouched), commit the changes and then change the contents of the file with a second commit? This way, I would expect git to recognize that the files were moved and not newly created. Wdyt?

Signed-off-by: Christoph Weitkamp <github@christophweitkamp.de>
Signed-off-by: Christoph Weitkamp <github@christophweitkamp.de>
Signed-off-by: Christoph Weitkamp <github@christophweitkamp.de>
@cweitkamp
Copy link
Contributor Author

@kaikreuzer I tried your proposed split.

The files are still shown as deleted in the full changes list. But the separate commits shows them as moved. Maybe GitHub cannot handle such a huge list of changes.

@Hilbrand
Copy link
Member

GitHub will show a broken history, even if you only move the file. Just browse any binding on the addons repo in relation to the move to bundle. You can still see the history via blame. That will show the original changes line by line.

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.

Ok, thanks - but I think it is definitely safer to keep the two separate commits in place as I am not convinced that git will correctly identify the moved files when they are changed in the same commit.
So the PR as it is now looks good to me and it should be merged by rebasing, not by squashing.

@openhab/core-maintainers Are you all fine with merging this or does anybody veto?

@wborn
Copy link
Member

wborn commented Dec 27, 2019

Let's go ahead with this and many thanks for working on this! I'm not that worried about blame/praise history being lost because there isn't that many to begin with since the ESH migration. I sometimes still go back there to see why code was written in a certain way.

@5iver
Copy link

5iver commented Dec 27, 2019

Where is the script located? I was curious to see if it was using git mv.

@cweitkamp
Copy link
Contributor Author

I would be fine with this part too. There are some minor other changes necessary to completely get rid of "ESH" (e.g. naming of folders for XML definitions ESH-INF/) or some properties like these:

@Component(service = ConfigDescriptionProvider.class, immediate = true, property = { "esh.scope=core.xml.binding" })

But I think that can be done in a follow-up PR.

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.

Remove usage of org.eclipse namespace
5 participants