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

Development of CUSTOM Printer Implementation with CommandValue Refactor #106

Draft
wants to merge 4 commits into
base: master
Choose a base branch
from

Conversation

jmalda
Copy link

@jmalda jmalda commented Feb 5, 2021

This PR builds on PR #105 and provides one possible way of creating an interface for CommandValues that can be implemented by different concrete implementations. This refactor touches a lot of files.

@igorocampos @danielmeza I look forward to your feedback and thoughts.

public interface ICommandValues
{
/* Barcodes */
byte PrintBarcode { get; }
Copy link
Author

Choose a reason for hiding this comment

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

One of the downsides of creating an interface like this is we lose the nice grouping of the different command values. For example, before we would reference something like Barcodes.SetBarWidth, whereas now it would be Values.SetBarWidth. There is no nice indication that this is a value that pertains to Barcodes.

We could preface the Values with Cmd or Barcodes so we use it like this: Values.CmdGS or Values.BarcodesSetBarWidth...but that doesn't feel quite right either. Any suggestions?

Copy link
Collaborator

Choose a reason for hiding this comment

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

Perhaps for the sake of simplicity just rename Values to Cmd or ByteCmd, I don't know... Something that can represent all of it.

But again, that's more of a decision that Luke would have to give his opinion about.

Copy link
Author

Choose a reason for hiding this comment

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

@lukevp Any thoughts on this?

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.

2 participants