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

[folderwatcher] Folder Watcher Binding initial contribution #4940

Closed
wants to merge 10 commits into from
Closed

[folderwatcher] Folder Watcher Binding initial contribution #4940

wants to merge 10 commits into from

Conversation

goopilot
Copy link
Contributor

@goopilot goopilot commented Feb 19, 2019

This is initial contribution of FTP FolderWatcher binding discussed here:
https://github.com/openhab/openhab2-addons/issues/4547 and here https://github.com/openhab/openhab2-addons/pull/4892
It's purpose is to monitor specific FTP(S) and local folder and trigger channel with new file names.

Fixes #4547

I'm asking community support to review code style and binding logic since it's my first binding and first experience with Java.

ToDo: SFTP, HTTP(S) folders monitoring

https://openhab.jfrog.io/openhab/libs-pullrequest-local/org/openhab/addons/bundles/org.openhab.binding.folderwatcher/2.5.5-SNAPSHOT/org.openhab.binding.folderwatcher-2.5.5-SNAPSHOT.jar

@goopilot goopilot requested a review from a team as a code owner February 19, 2019 18:15
@Hilbrand
Copy link
Member

Just don't open a new pull request everytime you have problems with git. It is totally unnecessary. Just fix you're branch or ask for help. Everytime you open a new pull request we have to look at all sources again and would loose any comments. For one thing it means I'm not going to review this PR anytime soon as I can't trust you keep reopening the pr costing me valuable time because I have to double check everything.

@goopilot
Copy link
Contributor Author

Sorry for that. That is my first time doing PR and working with git. I'm done with messing around now. No more adjustments unless they are requested.

@wborn wborn added the new binding If someone has started to work on a binding. For a new binding PR. label Feb 20, 2019
@wborn wborn changed the title [FolderWatcher] Initial contribution [folderwatcher] Folder Watcher Binding initial contribution Feb 20, 2019
@@ -0,0 +1,13 @@
# FIXME: please substitute the xx_XX with a proper locale, ie. de_DE
Copy link
Member

Choose a reason for hiding this comment

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

This file does nothing

@@ -0,0 +1,137 @@
<?xml version="1.0" encoding="UTF-8"?>
Copy link
Member

Choose a reason for hiding this comment

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

Wrong comments and indentation

Copy link
Contributor Author

Choose a reason for hiding this comment

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

fixed



<!-- Sample Thing Type -->
<thing-type id="localfolder">
Copy link
Member

Choose a reason for hiding this comment

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

Does it make sense to have specfic types for local and ftp files?
The binding could support arbitrary types of folders using URLs as configuration.

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Perhaps. That will require URL parser to extract configuration parameters.

Copy link
Member

Choose a reason for hiding this comment

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

Yep. This is more of an architectural decision, your choice.
(You could find a URL parser in java.net.URL)

@Component(configurationPid = "binding.folderwatcher", service = ThingHandlerFactory.class)
public class FolderWatcherHandlerFactory extends BaseThingHandlerFactory {

// = Collections.singleton(THING_TYPE_FTPFOLDER)
Copy link
Member

Choose a reason for hiding this comment

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

comment?


@Override
public void handleCommand(ChannelUID channelUID, Command command) {
if (CHANNEL_FILENAME.equals(channelUID.getId())) {
Copy link
Member

Choose a reason for hiding this comment

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

useless

throws IOException {
String dirToList = parentDir;
Calendar fileTimestamp = null;
Calendar cal = Calendar.getInstance();
Copy link
Member

Choose a reason for hiding this comment

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

don't use Calendar. Use java.time.Instant

ftp.logout();
ftp.disconnect();
} catch (IOException e) {
logger.debug("Error terminating FTP connection");
Copy link
Member

Choose a reason for hiding this comment

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

log the exception

// do nothing
}
}
logger.debug("Can't conect: {}", e.getMessage());
Copy link
Member

Choose a reason for hiding this comment

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

Use explicit exception logging

Copy link
Member

Choose a reason for hiding this comment

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

no need for "{}", e.getMessage(), just: logger.debug("Can't connect", e)

triggerChannel(CHANNEL_FILENAME, newFtpFile);

try {
Thread.sleep(3000);
Copy link
Member

Choose a reason for hiding this comment

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

same as with the local watcher

@goopilot
Copy link
Contributor Author

goopilot commented Feb 28, 2019

@t-8ch Thanks for all your comments, what's my next step? Do I fix my code and push it to my git? How will you know that it's being addressed?

@t-8ch
Copy link
Member

t-8ch commented Feb 28, 2019

You fix your code and push the new commit to your master branch on GitHub. It will be added automatically to this PR.

Keep this PR open!

When you are done just write a message.

@Nullable
private FolderLocalWatcherConfiguration config;
@Nullable
private WatcherCommon FileTools = new WatcherCommon();
Copy link
Member

Choose a reason for hiding this comment

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

This is not nullable, right? (It will go away anyways when making the WatcherCommon methods static.

@Nullable
private ScheduledFuture<?> executionJob;
@Nullable
private ArrayList<String> currentLocalListing = new ArrayList<String>();
Copy link
Member

Choose a reason for hiding this comment

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

The two arrays should not be nullable, no?

Copy link
Contributor Author

Choose a reason for hiding this comment

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

fixed

try {
previousLocalListing = FileTools.InitStorage(currentLocalListingFile);
} catch (IOException e) {
logger.debug("Can't write file {}.", currentLocalListingFile);
Copy link
Member

Choose a reason for hiding this comment

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

log the exeption

TimeUnit.SECONDS);
});
} else {
updateStatus(ThingStatus.OFFLINE, ThingStatusDetail.CONFIGURATION_ERROR);
Copy link
Member

Choose a reason for hiding this comment

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

Add a message, which configuration is broken.

triggerChannel(CHANNEL_LOCALFILENAME, newLocalFile);

try {
Thread.sleep(3000);
Copy link
Member

Choose a reason for hiding this comment

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

How did your rule access the filename?
If it gets it from the trigger event directly it should work (I think)

currentLocalListingFile);
} catch (IOException e1) {

logger.debug("Error geeting new file names");
Copy link
Member

Choose a reason for hiding this comment

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

Log the exception.

private ScheduledFuture<?> executionJob, initJob;
@Nullable
private FTPClient ftp;
@Nullable
Copy link
Member

Choose a reason for hiding this comment

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

These seem not be be nullable

try {
previousFtpListing = FileTools.InitStorage(currentFtpListingFile);
} catch (IOException e) {
logger.debug("Can't write file {}.", currentFtpListingFile);
Copy link
Member

Choose a reason for hiding this comment

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

exception logging

TimeUnit.SECONDS);
});
} else {
updateStatus(ThingStatus.OFFLINE, ThingStatusDetail.CONFIGURATION_ERROR);
Copy link
Member

Choose a reason for hiding this comment

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

Message for the status

// do nothing
}
}
logger.debug("Can't conect: {}", e.getMessage());
Copy link
Member

Choose a reason for hiding this comment

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

no need for "{}", e.getMessage(), just: logger.debug("Can't connect", e)

@t-8ch
Copy link
Member

t-8ch commented Mar 3, 2019 via email

@goopilot
Copy link
Contributor Author

goopilot commented Mar 3, 2019

All triggerevents should end up in events.log. Can you see it there correctly?

Yes, I saw it in events.log correctly.

@goopilot
Copy link
Contributor Author

goopilot commented Mar 3, 2019

@t-8ch fixed most of comments. Please tell me if I'm going into right direction.

try {
previousLocalListing = WatcherCommon.initStorage(currentLocalListingFile);
} catch (IOException e) {
logger.debug("Can't write file: {} error: {}", currentLocalListingFile, e);
Copy link
Member

Choose a reason for hiding this comment

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

Don't log the error explicitly.

Copy link
Contributor Author

Choose a reason for hiding this comment

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

@t-8ch Previously it was suggested to do explicit logging. Could you clarify please?

Copy link
Member

Choose a reason for hiding this comment

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

Ah sorry. For exceptions always use the implicit last argument

logger.debug("foo: {}", someObject, exception);

No need for a placeholder there.

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Fixed

@goopilot
Copy link
Contributor Author

goopilot commented Mar 5, 2019

Finished fixing latest remarks

currentLocalListing.addAll(listFiles(RootDir, config.listHiddenLocal, config.listRecursiveLocal));
} catch (IOException e) {

logger.debug("Can't read directory: ", config.localDir, e);
Copy link
Member

Choose a reason for hiding this comment

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

This logging is missing one format parameter

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Done

try {
previousFtpListing = WatcherCommon.initStorage(currentFtpListingFile);
} catch (IOException e) {
logger.debug("Can't write file: ", currentFtpListingFile, e);
Copy link
Member

Choose a reason for hiding this comment

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

logging parameter

Copy link
Contributor Author

Choose a reason for hiding this comment

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

@t-8ch done

@goopilot
Copy link
Contributor Author

@t-8ch did you have a chance to review latest changes? I hope it looks better now.

@t-8ch
Copy link
Member

t-8ch commented Mar 22, 2019

@goopilot Yep, looks fine to me.

@goopilot
Copy link
Contributor Author

@goopilot Yep, looks fine to me.

What it the next step for me?

@t-8ch
Copy link
Member

t-8ch commented Mar 25, 2019

@goopilot Now you are in the same boat as me :-)
We have to wait for one of the maintainers to review and approve it.

@goopilot
Copy link
Contributor Author

@t-8ch Thanks for quick response.

@openhab-bot
Copy link
Collaborator

This pull request has been mentioned on openHAB Community. There might be relevant details there:

https://community.openhab.org/t/folderwatcher-folder-watcher-binding/71149/1

Copy link
Member

@davidgraeff davidgraeff left a comment

Choose a reason for hiding this comment

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

thanks. I have left some inline comments.

Unfortunately you need to migrate this binding to the new buildsystem before we can merge. See the sticky issue for more information.

</parameter>
</config-description>
</thing-type>
</thing:thing-descriptions>
Copy link
Member

Choose a reason for hiding this comment

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

Have a look for missing newlines. You will see them as icon in the files changed tab here on github.

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Done

return new FolderWatcherHandler(thing);
}

if (THING_TYPE_LOCALFOLDER.equals(thingTypeUID)) {
Copy link
Member

Choose a reason for hiding this comment

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

should be "else if"

Copy link
Contributor Author

Choose a reason for hiding this comment

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

done

private final Logger logger = LoggerFactory.getLogger(FolderLocalWatcherHandler.class);

@Nullable
private FolderLocalWatcherConfiguration config;
Copy link
Member

Choose a reason for hiding this comment

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

please use the pattern private @Nullable ...

Copy link
Contributor Author

Choose a reason for hiding this comment

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

done

File[] fileList;
ArrayList<String> dirList = new ArrayList<String>();

// create new file
Copy link
Member

Choose a reason for hiding this comment

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

Don't comment the obvious. remove those single line comments and add a javadoc to the method instead and explain what is going on here (as this is a central method anyway)

Copy link
Contributor Author

Choose a reason for hiding this comment

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

done, Javadoc still todo

try {
currentLocalListing.addAll(listFiles(RootDir, config.listHiddenLocal, config.listRecursiveLocal));
} catch (IOException e) {

Copy link
Member

Choose a reason for hiding this comment

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

remove empty line

Copy link
Contributor Author

Choose a reason for hiding this comment

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

done

}

for (String newLocalFile : diffLocalListing) {

Copy link
Member

Choose a reason for hiding this comment

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

remove empty line

Copy link
Contributor Author

Choose a reason for hiding this comment

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

done

try {
WatcherCommon.saveNewListing(diffLocalListing, currentLocalListingFile);
} catch (IOException e2) {
logger.debug("Can't save new listing into file: ", e2);
Copy link
Member

Choose a reason for hiding this comment

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

If you just want the message of an exception use e2.getMessage(). Sometimes that makes sense.
If the exception is really exceptionally like in unexpected you want the exception object itself like you have done it here, so that the stacktrace is logged.

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Done

@davidgraeff
Copy link
Member

@t-8ch You said you are in the same boat, what is your binding that requires a review?

@t-8ch
Copy link
Member

t-8ch commented Apr 2, 2019

@davidgraeff #4629, you already reviewed it earlier today :-)

@davidgraeff davidgraeff self-assigned this Apr 2, 2019
@goopilot
Copy link
Contributor Author

@davidgraeff Sorry for long delay. I have problems migrating to new buildsystem so I'm thinking of simply setup new Eclipse IDE and copy-pasted source code of this binding. It's not so much in the end.
What would be the latest instruction of setting up Eclipse? The one at https://www.openhab.org/docs/developer/ide/eclipse.html says "We are currently reworking how to setup a perfect development environment. A new step by step guide will appear here soon." Can I still use it?

@kaikreuzer
Copy link
Member

@goopilot https://www.openhab.org/docs/developer/ide/eclipse.html is up-to-date and should provide you a working IDE setup.
For infos on how to best migrate to the new build system, please check https://github.com/openhab/openhab2-addons/issues/5005#issuecomment-467973902.

@TravisBuddy
Copy link

Travis tests were successful

Hey @goopilot,
we found no major flaws with your code. Still you might want to look at this logfile, as we usually suggest some optional improvements.

@goopilot
Copy link
Contributor Author

@davidgraeff I think I finally migrated to new binding system. What will be the next step?

@J-N-K
Copy link
Member

J-N-K commented Dec 31, 2019

It seems that this got out of sight. @goopilot are you willing to finish this one? If so, I can do review in the next days.

@goopilot
Copy link
Contributor Author

goopilot commented May 3, 2020

Ok, should be better now

*/
@NonNullByDefault
public class FtpFolderWatcherConfiguration {
public String ftpAddress = new String();
Copy link
Contributor

Choose a reason for hiding this comment

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

Don't use new String() instead just use an empty string literal. Please update all the others as well.

Suggested change
public String ftpAddress = new String();
public String ftpAddress = "";

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Fixed

*/
@NonNullByDefault
public class LocalFolderWatcherConfiguration {
public String localDir = new String();
Copy link
Contributor

Choose a reason for hiding this comment

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

Suggested change
public String localDir = new String();
public String localDir = "";

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Fixed

Comment on lines 163 to 167
if (config.secureMode.equals("NONE")) {
ftp = new FTPClient();
} else {
switch (config.secureMode) {
case "NONE":
ftp = new FTPClient();
break;
case "IMPLICIT":
ftp = new FTPSClient(true);
break;
case "EXPLICIT":
ftp = new FTPSClient(false);
break;
}
}
Copy link
Contributor

Choose a reason for hiding this comment

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

Suggested change
if (config.secureMode.equals("NONE")) {
ftp = new FTPClient();
} else {
switch (config.secureMode) {
case "NONE":
ftp = new FTPClient();
break;
case "IMPLICIT":
ftp = new FTPSClient(true);
break;
case "EXPLICIT":
ftp = new FTPSClient(false);
break;
}
}
switch (config.secureMode) {
case "NONE":
ftp = new FTPClient();
break;
case "IMPLICIT":
ftp = new FTPSClient(true);
break;
case "EXPLICIT":
ftp = new FTPSClient(false);
break;
}

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Fixed

Comment on lines 222 to 226
return;
} catch (IOException e) {
updateStatus(ThingStatus.OFFLINE, ThingStatusDetail.COMMUNICATION_ERROR, e.getMessage());
}
return;
Copy link
Contributor

Choose a reason for hiding this comment

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

These return statements aren't needed since you are already at the end of the method.

Suggested change
return;
} catch (IOException e) {
updateStatus(ThingStatus.OFFLINE, ThingStatusDetail.COMMUNICATION_ERROR, e.getMessage());
}
return;
} catch (IOException e) {
updateStatus(ThingStatus.OFFLINE, ThingStatusDetail.COMMUNICATION_ERROR, e.getMessage());
}

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Fixed

}

private void connectionKeepAlive() {
if (ftp.isConnected() == false) {
Copy link
Contributor

Choose a reason for hiding this comment

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

Suggested change
if (ftp.isConnected() == false) {
if (!ftp.isConnected()) {

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Fixed

private void refreshFolderInformation() {
final String rootDir = config.localDir;
try {
currentLocalListing.clear();
Copy link
Contributor

Choose a reason for hiding this comment

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

To avoid any concurrency issues I would suggest just creating a new list each time instead of trying to reuse the old instance.

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Fixed

if (!ftpRootDir.startsWith("/")) {
ftpRootDir = "/" + ftpRootDir;
}
currentFtpListing.clear();
Copy link
Contributor

Choose a reason for hiding this comment

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

same here

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Fixed

long diff = ChronoUnit.HOURS.between(file.getTimestamp().toInstant(), dateNow);

if (diff < config.diffHours) {
file.setName(dirToList + "/" + currentFileName);
Copy link
Contributor

Choose a reason for hiding this comment

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

I'm not too familiar with the FTP library so can you tell me why you are setting the file name here?

Copy link
Contributor Author

Choose a reason for hiding this comment

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

not sure, probably left-overs from troubleshooting

@TravisBuddy
Copy link

Travis tests were successful

Hey @goopilot,
we found no major flaws with your code. Still you might want to look at this logfile, as we usually suggest some optional improvements.

@@ -239,7 +231,7 @@ private void refreshFTPFolderInformation() {
if (!ftpRootDir.startsWith("/")) {
ftpRootDir = "/" + ftpRootDir;
}
currentFtpListing.clear();
List<String> currentFtpListing = new ArrayList<>();
Copy link
Contributor

Choose a reason for hiding this comment

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

You also need to call this.currentFtpListing = currentFtpListing; some where later in this method, or is the currentFtpListing field no longer needed?

Copy link
Contributor Author

@goopilot goopilot May 5, 2020

Choose a reason for hiding this comment

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

Yes, this is only used in the current function, so i removed global declaration and will only keep it locally

@@ -109,7 +108,7 @@ public void dispose() {
private void refreshFolderInformation() {
final String rootDir = config.localDir;
try {
currentLocalListing.clear();
List<String> currentLocalListing = new ArrayList<>();
Copy link
Contributor

Choose a reason for hiding this comment

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

same here

Copy link
Contributor Author

@goopilot goopilot May 5, 2020

Choose a reason for hiding this comment

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

Yes, this is only used in the current function, so i removed global declaration and will only keep it locally


== Source Code

https://github.com/openhab/openhab2-addons
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
https://github.com/openhab/openhab2-addons
https://github.com/openhab/openhab-addons

@NonNullByDefault
public class FtpFolderWatcherConfiguration {
public String ftpAddress = "";
public int ftpPort;
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
public int ftpPort;
public int ftpPort = 21;

also applies to the other parameters with default value

Copy link
Contributor Author

@goopilot goopilot May 5, 2020

Choose a reason for hiding this comment

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

@J-N-K do I really need to do this? Isn't it redundant? What's the point of having default value in thing-types.xml then? I don't see this in other bindings.
I've just tested it. When I create thing without specifying ftpPort at all it still sets to 21 even with "public int ftpPort;"

Copy link
Member

Choose a reason for hiding this comment

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

Which don‘t? Should be fixed there. The compiler can‘t know that and should show a warning if it is left uninitialized.

Copy link
Contributor Author

Choose a reason for hiding this comment

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

https://github.com/openhab/openhab-addons/blob/2.5.x/bundles/org.openhab.binding.amazondashbutton/src/main/java/org/openhab/binding/amazondashbutton/internal/config/AmazonDashButtonConfig.java
or
https://github.com/openhab/openhab-addons/blob/2.5.x/bundles/org.openhab.binding.airquality/src/main/java/org/openhab/binding/airquality/internal/AirQualityConfiguration.java
I'm sure there are more

On the other hand some other bindings have initialization, so I'm confused.
As I mentioned above even without specifying parameter value in thing config it is using default values as per thing-types.xml. Maybe openhab-core is handling this?

Anyway please let me know the final decision. I'll adjust accordingly.

Copy link
Contributor Author

Choose a reason for hiding this comment

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

@J-N-K did you have a chance to look at it?

Copy link
Contributor Author

Choose a reason for hiding this comment

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

@J-N-K I'm waiting for a decision on this. Can we have another pair of eyes to review this? I don't think we need to initialize variables here since we have default values in thing-types.xml.

"Polling interval can't be null or negative");
}

if (!config.secureMode.matches("(?i)NONE|IMPLICIT|EXPLICIT")) {
Copy link
Member

Choose a reason for hiding this comment

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

Copy link
Contributor Author

Choose a reason for hiding this comment

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

@J-N-K great, in that case I won't need this check at all

"Connection timeout can't be negative");
return;
}
if (config.ftpPort < 0) {
Copy link
Member

Choose a reason for hiding this comment

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

if you place limits on the parameter in the XML, you can skip the check here (also below)

Copy link
Contributor Author

Choose a reason for hiding this comment

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

@J-N-K this doesn't work for some reason. Maybe if I add thing via Paper UI that will work, but when I add it via config file I can enter negative value and while debugging I can see that value as negative as well.

Copy link
Member

Choose a reason for hiding this comment

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

IMO this is a bug in openhab-core then.

ftp.setConnectTimeout(config.connectionTimeout * 1000);

try {
if (config.ftpPort > 0) {
Copy link
Member

Choose a reason for hiding this comment

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

the default is automatically used, so you can always ftp.connect(config.ftpAddress, config.ftpPort);

return;
}

if (config.pollIntervalLocal > 0) {
Copy link
Member

Choose a reason for hiding this comment

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

see above

<description>Address of FTP server</description>
<context>network-address</context>
</parameter>
<parameter name="ftpPort" type="integer" min="1" max="65535">
Copy link
Member

Choose a reason for hiding this comment

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

I see you already have the limits set. The framework makes sure that these limits are observed.

<description>Allow listing of sub folders</description>
<advanced>true</advanced>
</parameter>
<parameter name="connectionTimeout" type="integer" min="1" unit="s">
Copy link
Member

Choose a reason for hiding this comment

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

see above, if there is a limit, no need to check that

Alexandr Salamatov added 10 commits July 7, 2020 01:19
Signed-off-by: Alexandr Salamatov <wpgnetworks@gmail.com>
Signed-off-by: Alexandr Salamatov <wpgnetworks@gmail.com>
Signed-off-by: Alexandr Salamatov <wpgnetworks@gmail.com>
Signed-off-by: Alexandr Salamatov <wpgnetworks@gmail.com>
Signed-off-by: Alexandr Salamatov <wpgnetworks@gmail.com>
Signed-off-by: Alexandr Salamatov <wpgnetworks@gmail.com>
Signed-off-by: Alexandr Salamatov <wpgnetworks@gmail.com>
Signed-off-by: Alexandr Salamatov <wpgnetworks@gmail.com>
Signed-off-by: Alexandr Salamatov <wpgnetworks@gmail.com>
POM
Signed-off-by: Alexandr Salamatov <wpgnetworks@gmail.com>
@TravisBuddy
Copy link

Travis tests have failed

Hey @goopilot,
please read the following log in order to understand the failure reason. There might also be some helpful tips along the way.
It will be awesome if you fix what is wrong and commit the changes.

@J-N-K
Copy link
Member

J-N-K commented Oct 31, 2020

@goopilot Any news here?

@J-N-K J-N-K added stale As soon as a PR is marked stale, it can be removed 6 months later. and removed awaiting feedback labels Dec 25, 2020
@kaikreuzer
Copy link
Member

Closing this PR as it is not in a mergeable state. Please follow the instructions to port this PR to the main branch for openHAB 3.

@kaikreuzer kaikreuzer closed this Jan 12, 2021
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
new binding If someone has started to work on a binding. For a new binding PR. stale As soon as a PR is marked stale, it can be removed 6 months later.
Projects
None yet
Development

Successfully merging this pull request may close these issues.

[folderwatcher] Add Folder Watcher Binding
10 participants