I want to cleanup and refactor this library (v40 proposal and discussion) #906
Comments
|
Hi, Forewordas I already suggested in other issues, I would try, whenever possible, to avoid behaviour by enums, favoring a strategy pattern instead. This would have the benefit to help (not solve) Topic 2 for example, moving specific configuration only where necessary. To be clear I would favour something like Topic 3This is the one I am mostly involved in. I agree that not allowing browsing the tree would help and that would also be strategy oriented. User provides the full path, it's up to the implementation use it as it is or navigate to the folder and then download Topic 4Having a simple Topic 6I am not a huge fan of async code, to put it mild. But I get this is a feature largely used nowadays, the only thing I could say is that at the moment I'm only using sync methods from Hope this helps |
|
Topic 3/4/5: Agreed (more or less) Forward;
It sounds pretty cool and could fix issues by offering simple and complex strategies. Assuming we have just one FtpClient for simplicity and all features are implemented in modular strategies, suggest the implementation. For example how would functionality be divided into strategies. For something like downloading a file, we don't really have strategies. Unless we should? Like |
|
I'm not expert on FluentFTP client internals, so I'm still talking in a very theoretical way, based mainly on the Find What Is Varying and Encapsulate It basic principle. public class FTPClient
{
public FTPClient(IDownloadStrategy downloadStrategy, IUploadStrategy uploadStrategy, IAuthenticationStrategy authStrategy)
{
// Of course, as many strategies as required
}
public void Download()
{
// You can du stuff here before calling the strategy, but it MUST be server independent, guaranteed
m_downloadStrategy.Download();
// Or the call could contain another strategy
m_downloadStrategy.Download(m_authStrategy);
/* This is a key point, strategies could have other strategies from constructor or at runtime,
it really depends on the scenario. Passing at runtime may allow different behaviour, there is no
good or bad, passing in construction gives the dev a message: this is readonly and cannot be
changed, so depending on the scenario may be the right choice */
}
}
public static class FritzBoxFtpClient
{
// Here custom properties for this kind of client, should help with cluttered config
// e.g. TransferChunkSize goes into the proper stategy
public static FTPClient Create()
{
return new FTPClient(); // Inject strategies for this specific client
}
}I do expect having, for each strategy type, a number N of implementations which is less than the actual number of clients, i.e. I guess a The other advantage is that this pattern would allow native extendibility. E.g. a user has a new client, maybe a custom one. He can create his own strategies and inject into Moreover, async is in the syntax, so it doesn't work, but a Download strategy could be one file at a time and another implementation may be The hard part is:
|
|
Just coming from my insular view for the IBM parts of this: I am worried about: Topic 3 Anyone writing a FTP Client GUI and wanting to give the user a way to walk the server-OS's file system will want to have these two functions, right? And the IBM parts really heavily use PWD/CWD. Topic 6
Same for me. I find it strange that C# religion goes for "not duplicating code" but makes it so difficult to have the same basic algorithm present in both sync and async modes. Certainly in an C(++) environment I could envision a single code set (using the preprocessor) that represents both flavors in a single source. How to do in C#? |
|
Topic 3
Maybe we can have a strategy for this?
Names are just suggestions, you can suggest better ones. I also don't like having the word "Strategy" as a suffix, is there a better naming scheme? For backwards compatibility, we could support all the older constructors, which would be implemented like: Topic 6
Same for me, however lots of contributors were excited to contribute async versions for some reason... And it makes the library look more "modern". I don't know why users can't just create a background worker/thread and use the sync API, maybe its because of the whole node.js "everything is an async/non-blocking operation" pattern that took the world by storm. |
robinrodricks
added
response requested
high-priority
and removed
response requested
labels
|
This may be silly or very naif. A |
No, that would not solve the main problem of duplicating code. |
|
I would like to see
|
I'm not against it but it caused #703 and so it was reverted. If you have a solution for it without causing that issue, you can please do it and file a PR. |
Ok but then this is why
If they could, we also could in an extension method, right? |
|
I don't like providing main library API as extension methods. Extensions are meant when the third parties want to extend something without inheriting it. |
Hi, Please take a look here: https://github.com/RobertoSarati/FluentFTP_asyncenum. (fork reset to release 33.1.6). Btw, do you still want to support net3.5, net4, net45? According to microsoft .net framework < 4.6.2 are no more supported. So, what if target framework list is just [netstandard2.0;netstandard2.1;net50+]? |
Ok that sounds cool @sierrodc , can you check if
That is what I am asking the community. For myself I don't mind as I only use the latest .NET framework. Unrelated: |
Imho the "sync/async" programming style should be as close as possible to what is today available in HttpClient so, for sure, no IAsyncResult. About sync method... I'm fine to have only aysnc/await (I'll use only these ones and I dont' want many methods in the intellisense dropdown), but if the community wants to keep sync style, I've no objections. But for me, we can switch to implement only async/await methods. |
|
What I've planned for v40: To delete
To refactor:
To add:
|
|
Do you guys think this is a decent solution? At present I don't see any advantage to the user for splitting the download/upload/FXP parts, because even if I did refactor the "downloader" to use strategy pattern, I don't have any alternate strategies to offer the user. It would just force the user to change from: |
|
1;3;4;7;8: ok
|
|
First of all thanks for raising this discussion. Topic 4: LoggingBuilding on top of the suggestion by @Adhara3 here's a rough prototype of how the logging could be structured Define an interface in interface IFluentFtpLogger
{
void Log(FtpTraceLevel eventType, object message);
}
class NoOpLogger : IFluentFtpLogger
{
public void Log(FtpTraceLevel eventType, object message)
{
// The default logger that does nothing.
}
}
class ConsoleLogger : IFluentFtpLogger
{
public void Log(FtpTraceLevel eventType, object message)
{
// Write to Console
throw new NotImplementedException();
}
}
class FileLogger : IFluentFtpLogger
{
public void Log(FtpTraceLevel eventType, object message)
{
// Write to file
throw new NotImplementedException();
}
}
/// <summary>
/// Allows combining multiple loggers
/// </summary>
class CompositeLogger : IFluentFtpLogger
{
private readonly IFluentFtpLogger[] m_loggers;
public CompositeLogger(params IFluentFtpLogger[] loggers)
{
m_loggers = loggers;
}
public void Log(FtpTraceLevel eventType, object message)
{
foreach (var logger in m_loggers)
{
logger.Log(eventType, message);
}
}
}To allow hooking into the widely used class LoggerAdapter : IFluentFtpLogger
{
private readonly ILogger<IFtpClient> logger;
public void Log(FtpTraceLevel eventType, object message)
{
var loglevel = ConvertLogLevel(eventType);
if (logger.IsEnabled(loglevel))
{
logger.Log(loglevel, message.ToString());
}
}
private static LogLevel ConvertLogLevel(FtpTraceLevel eventType)
{
return eventType switch
{
FtpTraceLevel.Verbose => LogLevel.Trace,
FtpTraceLevel.Info => LogLevel.Information,
FtpTraceLevel.Warn => LogLevel.Warning,
FtpTraceLevel.Error => LogLevel.Error,
_ => LogLevel.Information // Fallback
};
}
}Topic 6: Target frameworksFluentFtp currently supports frameworks that have been EOL for years.
I we want to support older but still supported frameworks, we could go with https://docs.microsoft.com/en-us/dotnet/standard/library-guidance/cross-platform-targeting Topic 6: Asyncasync is important in modern C# to avoid thread starvation, so I highly recommend not to drop that part. In regards to whether we should also keep the sync APIs, other clients have mostly dropped sync versions.
Topic 7: Removing
|
|
@jnyrup Thanks for the long and detailed answer! Topic 4: LoggingI dislike "too many nuget packages" so would it cause any harm to add a direct dependency on And then do as you proposed above. (NoOpLogger, FileLogger...) Maybe one change would be instead of having a single instance of logger, we could have a list of loggers directly in the client for simplicity? Making API would be: Topic 6: Target frameworksI agree (mostly): Current frameworks: Proposed frameworks: Topic 6: AsyncI am planning on splitting ConcurrencyYes concurrency is not currently supported. If you try to do multiple simultaneous operations on a single FTP client connection it will break (as per limitations of FTP protocol). |
|
I would probably drop the public interface IFtpLogger
{
void Trace(string message); // I personally see no advantages in passing an object
void Debug(string message);
void Info(string message);
void Warning(string message);
void Error(string message);
void Error(string message, Exception exception);
}I would also probably provide only the composite and the
I would not insist any further on this, I just wanted to clarify that:
ConcurrencyThis is interesting. As a user, the async interface would give me the natural feeling that the client is concurrent, so be careful here. As suggested by @jnyrup is a user expect to run the client as the A |
Topic 4: Logging
In terms of functionality of FluentFtp taking a direct dependency on M.E.L.A should "just work", but in general there are more considerations to take into account when adding extra dependencies. When I'm about to add an additional nuget package, I have to evalute all transitive package references, in order to get a full view on licenses, vulnerabilities, maintenance status, etc. to avoid creating technical debt/security issues from day one. Increased size of your application as extra unused dlls are included. Note, the earliest non-deprecated of this nuget package targets netstandard20 which is net461 or later. So I think it mostly comes down to ones view on providing a single package with all batteries included or a more modular approach, where consumers don't have to depend on something they don't use.
Established logging frameworks already provide knobs to combine multiple loggers (sinks) into a single As an endorsement, the general "best-practice" is to use constructor-based dependency injection, but since a So I would probably go with Two benefits of using the methods exposed on
Topic 6: Target frameworks
I shouldn't be necessary to have both net50 and net60, unless you want to use net60 specific features or import different nuget packages based on the framework, as a net60 applications can consume a net50 nuget. Out of curiosity, why do you intend to keep support for net35, net40 and net45? Topic 6: Async
I wouldn't do that. public interface IAsyncFtpClient
{
public Task<byte> DownloadAsync();
}
public interface ISyncFtpClient
{
public byte Download();
}
public class FtpClient : IAsyncFtpClient, ISyncFtpClient
{}Strategies
I agree here that ideally, the |
Topic 4: LoggingAgreed on MELA. I prefer "batteries included" as far as possible so I would like to implement all these in FluentFTP itself:
Topic 6: Target frameworks
Well just to support a broader user base. I was aware of enterprises that find it harder to upgrade to newer .NET versions. What is your take on this? If we are to support MELA then we would be forced to drop net 3.5/4/4.5 which is ok with me provided its ok with the user base. I specifically wanted net5/6 so that users with those 2 frameworks can use FluentFTP without forcing you to upgrade to THE latest version. For example some businesses don't want to keep re-purchasing the latest VS version. Topic 6: Async
Would anyone do that? A lot of the users on this thread mentioned they use either sync or async but not both. I can't imagine how you would use both, considering that FluentFTP does not allow concurrent operations anyway. In fact I specifically don't like this method of mixing sync and async code as it may not be well tested usage.
Is it ok to drop the 'Async' suffix for async methods?From 1:
From 2:
Strategies
We already have implemented a server-specific strategy system which implements this as far as possible. Some zOS specific code is still remaining in the core which should be refactored out as far as possible. |
Topic 6: Target frameworks
MS ended support for .net2.0 over 10 years ago and for net40 over 5 years ago.
.NET5 is targeted in FluentFTP to add support for TLS 1.3. Topic 6: AsyncI think I understand your rationale now, to help users not mixing sync and async calls. |
robinrodricks
changed the title
robinrodricks
changed the title
|
I tried out 40.0.0-beta3 on the handful of different FTP servers we connect to, and I didn't notice any problems. |
|
@jnyrup Awesome, thanks very much for testing! I have released a nuget with your changes, I would appreciate if you could test again. https://www.nuget.org/packages/FluentFTP/40.0.0-beta4 I added a note for all the contributors who made PRs against v40. Thanks guys! @FanDjango Once you confirm that beta4 is good, I will release v40! |
|
@robinrodricks I confirm 24dba70 works for my stuff One thing disturbs me after this sequence of events: My test-suite uses a CLI calling the base of my app, it is a Console App, and I observe and check/analyze its output. My actual production app is a Win Forms GUI. The test-suite (obviously) went a different compile path (#if...) on build and worked. This told me, all is well, and then one of my clients, an important one but also one of the best - I love it when they pick up your updates and you get the feeling that they are "panting" for new stuff, reported this unexpected failure. Do you have any ideas how to address this discrepancy? I mean to say that the growing test suite is certainly also subject to this kind of divergence, not just mine. Also, I cannot put a z/OS FTP server into a docker container - shouldn't we have a "sample" test unit for a "sample" real FTP server, of whatever kind. I would use that one... |
|
This business concerning Apart from the details of the previous discussion, I see the following: Look, example:
You are going to see some stuff anyway. Configure the server, of course, but you cannot guarantee? Wow, what privacy. But that's not my desire, to hide the user. `Config.LogCredentials' (tries) to hide both, but then: see 331 and 230. How can I enable logging of the I mean, all I want to hide is the password? Of course, I can "correct" that in my logger routine (whichever method I choose to use). |
Both are the same platform? .NET 4.7?
What failure?
It is possible some are wrong, but I double checked it, and it looked ok.
Lol. I will add back User/Pass config. I will try to hide |
Oooops.
After some checking, I am no longer disturbed. |
If some platform fails, I need to know. |
@FanDjango Done, please check the latest master - |
Yes, but you fixed it already, no longer a problem. But I found a problem on my side - testing with two different targets, a simple typo, which now both work, finally.
Doing that in a moment |
|
Latest master works for me. LogUserName and LogPassword: works also |
|
Should I release the v40 nuget @FanDjango and @jnyrup ? |
|
I have only a few minor comments left, but only the first one might be blocking v40 as it changes the API. From the C# Design Guidelines
There are 77 violations of that guideline list of files
Unused return values |
Yes I never understood why C# forced the slower properties to be used instead of public fields. It actually has a negative effect on run time performance (may be negligible). Most of the uses here are just plain POCOs which don't really need properties. |
|
A benefit of properties is that in debug mode, you can set breakpoints on them. In release mode the call to the property is inlined, so the generated assembly is identical whether you use a field with or without a an explicit backing field. |
|
@jnyrup Ok, agreed that we should follow guidelines as it makes the lib more professional. I fixed all issues except for the last 2 (return values), they are valid, or I cannot figure it out. Release https://www.nuget.org/packages/FluentFTP/40.0.0-beta5 Guys please check and confirm it works, so I can release the final nuget. |
Something one takes for granted until you need to find out from whence came an update, or can you not break-point on a value (i.e. a field) changing its value?
This is a profoundly valuable statement that reconciles me with the tediousness of making "quasi" unneeded properties. |
|
@robinrodricks Newest master works for me One point: If I compare, file by file, the Async client with the Sync client, there are subtle differences in the code. See the following screenshot of ExamDiff on
There are actually a few of these, some only the comments, but some more than just that. Worth looking at, file by file (61 files)? |
I am just a single one of your users. Who am I to decide. You are raring to go and the changes are sensible, the code is much nicer this way, it becomes much easier (See my "ExamDiff" on previous post) to see any SYNC-ASYNC discrepancies, much less "#if"finess in the code. I needed a small amount of code modifications. Users might gripe and grouch, but they gain a logging framework or can use legacy logging, they get cleaner code leading to better maintenance. If you follow through with handling their issues, they will see the upside of the V39-V40 switch. So ok, go for it... IMHO So, I am finished with this thread and will move on to other topics on my Todo-List. I thank all for the fruitful discussions and I learned a lot also. |
Thank you!
Seems like a bug.
You can go ahead if you have time, sorry I won't have time this week. Beyond Compare would make quick work of it.
I'll wait some days and then release. |
I was not aware of this. |
|
Published v40! https://www.nuget.org/packages/FluentFTP/40.0.0 Now to wait for the coming storm (of issues!) Thanks to ALL who participated in this thread. I am closing it, we can take the remaining features or any new issues as part of new discussions. |
Intro
Here are things that I'd like to cleanup going forward. Since this has ramifications on all existing projects using FluentFTP, and since these will be major breaking changes to the API, I'd like community feedback on these changes so that they are not hasty, disruptive, or out-of-touch with the actual userbase.
I would like these to be part of the v40 release so its easy for existing users to understand that v39 is the "legacy API" and that v40+ will use the "newer API". I have never undertaken such a large systematic refactoring before.
Topic 1
EnableThreadSafeDataConnections- Crazy feature which spawns new FTP connections for each file transferredQuickTransferLimit- Implemented but disabledFtpClient.Connect(Uri)- Static constructorsFtpClient.GetPublicIP()- Static methodsDereferenceLink- does anyone even use this?FtpClient- I would just like 1 or 2 constructors for common patterns.Topic 2
FtpClientclass which clutters up its API.FtpClient. This groups up the properties into logical categories and also eliminates them from the FtpClient API.client.Connect()becomesclient.Connect(FtpProfile)client.FXPProgressIntervalbecomesclient.FXP.ProgressIntervalclient.DownloadZeroByteFilesbecomesclient.Download.ZeroByteFilesclient.TransferChunkSizebecomesclient.Download.ChunkSizeandclient.Upload.ChunkSizeTopic 3
/root/animals/duck.txt, therefore we should always set the working directory to the folder (eg:/root/animals/) and then download the file (duck.txt). And we should not allow the user to manually modify the working directory because it would conflict with this system.GetWorkingDirectoryandSetWorkingDirectoryand instead always automatically set the working directory before file transfers or even getting the file size, etc. This would even fix issues where there are spaces in directory paths, and many other things. It would put us in parity with FileZilla.Topic 4
FtpTraceand its outdated API and instead support aclient.Loggerproperty which can be set to our default logger or an NLog instance, etc.Topic 5
NoTimeConversionTimeZoneConversion- a new method that allows for DST to be observed (missing feature)TimeOffsetConversion- users specify which is their PC timezone and the server timezone and we do simple math to convert it (current method)Topic 6
Topic 7
IAsyncResultstyle async functions.async/await.Lets talk!
Please start the discussions below! Add your comments and mention which topic you are talking about. You can comment on any topic(s) that you like.
The text was updated successfully, but these errors were encountered: