Skip to content

Dev mode for console sender #70

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

Closed
wants to merge 2 commits into from
Closed

Conversation

arggh
Copy link

@arggh arggh commented Nov 4, 2018

Addressing issue #42 , I'm suggesting this change to the ConsoleSender to enable nice and short, yet informative, console messages when developing.

I followed the convention of other senders in how to handle function arguments, but I'd personally prefer named args.

Then, this:

const shortSender = new ConsoleSender(true);

would become this:

const shortSender = new ConsoleSender({ devMode: true });

...which I think is a lot better and self-documenting. Should I make the change?

@arggh
Copy link
Author

arggh commented Jan 6, 2019

I'ts been a while, could you perhaps consider merging this or describe what should be changed?

@fgm
Copy link
Owner

fgm commented Feb 28, 2019

Just found time to look at the PR: seems nice enough. I have no real preference for options being an object or inline arguments. 4 things to consider:

  • AIUI jsdoc needs a @Property annotation for devMode could
  • It should have a unit test attached testing the change in behavior
  • It will need a CHANGELOG entry since it modifies the default behaviour
  • the master branch is currently 0.1.x ; at some point hopefully rather soon, PR Apply context sourcing on the TypeScript version #67 will be merged, introducing version 0.2.x, and this patch will then need to be ported

@fgm fgm closed this Jan 12, 2021
@fgm fgm deleted the branch fgm:master January 12, 2021 12:31
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