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

♻️ Another pass at reducing complexity around Log #35461

Merged
merged 13 commits into from
Aug 9, 2021

Conversation

rcebulko
Copy link
Contributor

@rcebulko rcebulko commented Jul 29, 2021

  • move setReportError to error-reporting
  • move user embed error message helpers to #core/error/message-helpers
  • general reduction in complexity and verbosity of some Log methods/constructor helpers
  • deleting methods that aren't used anywhere

@@ -211,7 +180,7 @@ export class Log {
}

// LocalDev by default allows INFO level, unless overriden by `#log`.
if (getMode().localDev && !getMode().log) {
if (getMode().localDev) {
Copy link
Contributor Author

Choose a reason for hiding this comment

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

getMode().log == 0 above implies !getMode().log already

@@ -224,46 +193,40 @@ export class Log {
*/
defaultLevelWithFunc_() {
// Delegate to the specific resolver.
return this.levelFunc_(parseInt(getMode().log, 10), getMode().development);
return this.levelFunc_(getMode().log, getMode().development);
Copy link
Contributor Author

Choose a reason for hiding this comment

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

moved parsing into mode

*/
isEnabled() {
return this.getLevel_() != LogLevel.OFF;
return true;
Copy link
Contributor Author

Choose a reason for hiding this comment

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

isEnabled wasn't used anywhere.


return this.messages_?.[id]
? [this.messages_[id]].concat(parts)
: [`More info at ${externalMessageUrl(id, parts)}`];
Copy link
Contributor Author

Choose a reason for hiding this comment

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

This code is mostly untouched, but I moved all the assert helpers to the end of the class definition so the rest of the log/error methods were together.

@rcebulko rcebulko changed the title ♻️ Another pass as reducing complexity around Log ♻️ Another pass at reducing complexity around Log Jul 30, 2021
@rcebulko rcebulko requested a review from jridgewell July 30, 2021 14:50
@rcebulko rcebulko marked this pull request as ready for review July 30, 2021 14:51
@amp-owners-bot
Copy link

Hey @jridgewell! These files were changed:

src/core/error/message-helpers.js

@rcebulko
Copy link
Contributor Author

rcebulko commented Aug 2, 2021

@jridgewell bumping this

src/mode.js Show resolved Hide resolved
@samouri
Copy link
Member

samouri commented Aug 2, 2021

@rcebulko you are my hero

}

const cs = this.win.console;
const fn =
Copy link
Member

Choose a reason for hiding this comment

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

clever, but IMO harder to read. not b/c the pattern is inherently worse, its just super uncommon


return this.messages_?.[id]
? [this.messages_[id]].concat(parts)
: [`More info at ${externalMessageUrl(id, parts)}`];
Copy link
Member

Choose a reason for hiding this comment

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

FMI: do you know if this code is actually active? I didn't realize we even had started on this feature

Copy link
Contributor Author

Choose a reason for hiding this comment

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

It has never been used/shipped, but was working (though untested) at one point. The transforms likely need updating since much of assertion code has been moved.

src/log.js Show resolved Hide resolved
src/log.js Show resolved Hide resolved
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Projects
None yet
Development

Successfully merging this pull request may close these issues.

3 participants