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

Library compatibility - Keep revision 1 as default #100

Closed
roberto-guerzoni opened this issue Apr 14, 2023 · 6 comments
Closed

Library compatibility - Keep revision 1 as default #100

roberto-guerzoni opened this issue Apr 14, 2023 · 6 comments

Comments

@roberto-guerzoni
Copy link

Good evening,
I always follow this project with pleasure and I noticed that in the latest release some mids have been updated. In order not to lose compatibility with the previous version, I think it's better not to always create the mid with the latest release, better to create it with the minor release and let the developer upgrade the release when needed.

@Rickedb Rickedb changed the title Library compatibility Library compatibility - Keep revision 1 as default Apr 16, 2023
@Rickedb
Copy link
Owner

Rickedb commented Apr 16, 2023

Hi @roberto-guerzoni, this will cause breaking changes.
However, I was thinking about it and most communications tend to be rev 1 and other revs are more like optional when you need a specific information.
I will consider it for the next version.

Thank you!

@roberto-guerzoni
Copy link
Author

roberto-guerzoni commented May 5, 2023

Hi Rickedb,
looking your commit #100 noticed this point might be wrong

public Mid0001(int revision = 6) : this(new Header() => public Mid0001(int revision = DEFAULT_REVISION) : this(new Header()

@roberto-guerzoni
Copy link
Author

roberto-guerzoni commented May 5, 2023

Hi Rickedb,
Could you consider this improvements?

Mid.cs

 protected virtual void ProcessDataFields(string package)
        {
            if (!RevisionsByFields.Any())
                return;

            int revision = Header.Revision > 0 ? Header.Revision : 1;
            for (int i = 1; i <= revision; i++)
            {
                if (RevisionsByFields.TryGetValue(i, out var field))
                {
                    ProcessDataFields(field, package);
                }
            }
        }

MidInterpreterMessagesExtensions.cs

public static MidInterpreter UseAllMessages(this MidInterpreter midInterpreter, IEnumerable<Type> mids)
        {
			var enumerable = mids.ToList();
			if (enumerable.Any(x => !x.IsSubclassOf(typeof(Mid))))
                throw new ArgumentException("All mids must inherit Mid class", nameof(mids));

            return midInterpreter
                .UseAlarmMessages(enumerable.Where(x => DoesImplementInterface(x, typeof(IAlarm))))
                .UseApplicationControllerMessage(enumerable.Where(x => DoesImplementInterface(x, typeof(IApplicationController))))
                .UseApplicationSelectorMessages(enumerable.Where(x => DoesImplementInterface(x, typeof(IApplicationSelector))))
                .UseApplicationToolLocationSystemMessages(enumerable.Where(x => DoesImplementInterface(x, typeof(IApplicationToolLocationSystem))))
                .UseAutomaticManualModeMessages(enumerable.Where(x => DoesImplementInterface(x, typeof(IAutomaticManualMode))))
                .UseCommunicationMessages(enumerable.Where(x => DoesImplementInterface(x, typeof(ICommunication))))
                .UseIOInterfaceMessages(enumerable.Where(x => DoesImplementInterface(x, typeof(IIOInterface))))
                .UseJobMessages(enumerable.Where(x => DoesImplementInterface(x, typeof(IJob))))
                .UseAdvancedJobMessages(enumerable.Where(x => DoesImplementInterface(x, typeof(IAdvancedJob))))
                .UseLinkCommunicationMessages(enumerable.Where(x => DoesImplementInterface(x, typeof(ILinkCommunication))))
                .UseMotorTuningMessages(enumerable.Where(x => DoesImplementInterface(x, typeof(IMotorTuning))))
                .UseMultipleIdentifiersMessages(enumerable.Where(x => DoesImplementInterface(x, typeof(IMultipleIdentifier))))
                .UseMultiSpindleMessages(enumerable.Where(x => DoesImplementInterface(x, typeof(IMultiSpindle))))
                .UseOpenProtocolCommandsDisabledMessages(enumerable.Where(x => DoesImplementInterface(x, typeof(IOpenProtocolCommandsDisabled))))
                .UseParameterSetMessages(enumerable.Where(x => DoesImplementInterface(x, typeof(IParameterSet))))
                .UsePLCUserDataMessages(enumerable.Where(x => DoesImplementInterface(x, typeof(IPLCUserData))))
                .UsePowerMACSMessages(enumerable.Where(x => DoesImplementInterface(x, typeof(IPowerMACS))))
                .UseResultMessages(enumerable.Where(x => DoesImplementInterface(x, typeof(IResult))))
                .UseStatisticMessages(enumerable.Where(x => DoesImplementInterface(x, typeof(IStatistic))))
                .UseTighteningMessages(enumerable.Where(x => DoesImplementInterface(x, typeof(ITightening))))
                .UseTimeMessages(enumerable.Where(x => DoesImplementInterface(x, typeof(ITime))))
                .UseToolMessages(enumerable.Where(x => DoesImplementInterface(x, typeof(ITool))))
                .UseUserInterfaceMessages(enumerable.Where(x => DoesImplementInterface(x, typeof(IUserInterface))))
                .UseVinMessages(enumerable.Where(x => DoesImplementInterface(x, typeof(Vin.IVin))));
        }

@Rickedb
Copy link
Owner

Rickedb commented May 14, 2023

Hi Rickedb, Could you consider this improvements?

Mid.cs

 protected virtual void ProcessDataFields(string package)
        {
            if (!RevisionsByFields.Any())
                return;

            int revision = Header.Revision > 0 ? Header.Revision : 1;
            for (int i = 1; i <= revision; i++)
            {
                if (RevisionsByFields.TryGetValue(i, out var field))
                {
                    ProcessDataFields(field, package);
                }
            }
        }

MidInterpreterMessagesExtensions.cs

public static MidInterpreter UseAllMessages(this MidInterpreter midInterpreter, IEnumerable<Type> mids)
        {
			var enumerable = mids.ToList();
			if (enumerable.Any(x => !x.IsSubclassOf(typeof(Mid))))
                throw new ArgumentException("All mids must inherit Mid class", nameof(mids));

            return midInterpreter
                .UseAlarmMessages(enumerable.Where(x => DoesImplementInterface(x, typeof(IAlarm))))
                .UseApplicationControllerMessage(enumerable.Where(x => DoesImplementInterface(x, typeof(IApplicationController))))
                .UseApplicationSelectorMessages(enumerable.Where(x => DoesImplementInterface(x, typeof(IApplicationSelector))))
                .UseApplicationToolLocationSystemMessages(enumerable.Where(x => DoesImplementInterface(x, typeof(IApplicationToolLocationSystem))))
                .UseAutomaticManualModeMessages(enumerable.Where(x => DoesImplementInterface(x, typeof(IAutomaticManualMode))))
                .UseCommunicationMessages(enumerable.Where(x => DoesImplementInterface(x, typeof(ICommunication))))
                .UseIOInterfaceMessages(enumerable.Where(x => DoesImplementInterface(x, typeof(IIOInterface))))
                .UseJobMessages(enumerable.Where(x => DoesImplementInterface(x, typeof(IJob))))
                .UseAdvancedJobMessages(enumerable.Where(x => DoesImplementInterface(x, typeof(IAdvancedJob))))
                .UseLinkCommunicationMessages(enumerable.Where(x => DoesImplementInterface(x, typeof(ILinkCommunication))))
                .UseMotorTuningMessages(enumerable.Where(x => DoesImplementInterface(x, typeof(IMotorTuning))))
                .UseMultipleIdentifiersMessages(enumerable.Where(x => DoesImplementInterface(x, typeof(IMultipleIdentifier))))
                .UseMultiSpindleMessages(enumerable.Where(x => DoesImplementInterface(x, typeof(IMultiSpindle))))
                .UseOpenProtocolCommandsDisabledMessages(enumerable.Where(x => DoesImplementInterface(x, typeof(IOpenProtocolCommandsDisabled))))
                .UseParameterSetMessages(enumerable.Where(x => DoesImplementInterface(x, typeof(IParameterSet))))
                .UsePLCUserDataMessages(enumerable.Where(x => DoesImplementInterface(x, typeof(IPLCUserData))))
                .UsePowerMACSMessages(enumerable.Where(x => DoesImplementInterface(x, typeof(IPowerMACS))))
                .UseResultMessages(enumerable.Where(x => DoesImplementInterface(x, typeof(IResult))))
                .UseStatisticMessages(enumerable.Where(x => DoesImplementInterface(x, typeof(IStatistic))))
                .UseTighteningMessages(enumerable.Where(x => DoesImplementInterface(x, typeof(ITightening))))
                .UseTimeMessages(enumerable.Where(x => DoesImplementInterface(x, typeof(ITime))))
                .UseToolMessages(enumerable.Where(x => DoesImplementInterface(x, typeof(ITool))))
                .UseUserInterfaceMessages(enumerable.Where(x => DoesImplementInterface(x, typeof(IUserInterface))))
                .UseVinMessages(enumerable.Where(x => DoesImplementInterface(x, typeof(Vin.IVin))));
        }

@roberto-guerzoni
First one is fine, we save some operations.

However, the second one doesn't seems to make much sense, why .ToList() and using Linq over IEnumerable<> if the parameter is already an IEnumerable<>?

@roberto-guerzoni
Copy link
Author

roberto-guerzoni commented May 14, 2023 via email

@Rickedb
Copy link
Owner

Rickedb commented May 15, 2023

Yes, that is true, if your IEnumerable<T> is generated using yield return for example, this will cause to re-enumerating every GetEnumerator() operation.

However, by doing this I'm forcing to materialize everything, also your operation at database, which, sometimes might lead to a problem too and we will just switch problems, and this behaviour would be forced without user`s acknowledgement.

Another possible option is to use IReadOnlyCollection<T> as parameter, to explicitly ensure that the provided collection is already materialized.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Projects
None yet
Development

No branches or pull requests

2 participants