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.

Sign up for GitHub

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

Jump to bottom

rocketchat-lib part1 #6553

Merged
merged 4 commits into from
Apr 4, 2017
Merged

rocketchat-lib part1 #6553

merged 4 commits into from
Apr 4, 2017

Conversation

ggazzo
Copy link
Member

@ggazzo ggazzo commented Mar 31, 2017

No description provided.

rodrigok
rodrigok requested changes Mar 31, 2017
View reviewed changes
packages/rocketchat-lib/lib/configLogger.js Outdated
@@ -0,0 +1,19 @@
/* globals LoggerManager */
RocketChat.settings.get('Log_Package', function(key, value) {
return (typeof LoggerManager !== 'undefined') && LoggerManager !== null && (LoggerManager.showPackage = value);
Copy link
Member

Choose a reason for hiding this comment

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

Simplify to LoggerManager && LoggerManager.showPackage = value;

Copy link
Member Author

Choose a reason for hiding this comment

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

actually, if the variable LoggerManager have never been declared, It will throw an exception

LoggerManager && LoggerManager.showPackage = value; // Uncaught ReferenceError: LoggerManager is not defined
typeof LoggerManager != 'undefined' && LoggerManager.showPackage = value; // nothing happens

packages/rocketchat-lib/lib/configLogger.js Outdated
});

RocketChat.settings.get('Log_File', function(key, value) {
return typeof LoggerManager !== 'undefined' && LoggerManager !== null && (LoggerManager.showFileAndLine = value);
Copy link
Member

Choose a reason for hiding this comment

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

Same here

packages/rocketchat-lib/lib/configLogger.js Outdated

RocketChat.settings.get('Log_Level', function(key, value) {
if (value != null) {
if (typeof LoggerManager !== 'undefined' && LoggerManager !== null) {
Copy link
Member

Choose a reason for hiding this comment

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

Simplify to LoggerManager != null

packages/rocketchat-lib/lib/configLogger.js Outdated
LoggerManager.logLevel = parseInt(value);
}
Meteor.setTimeout(() => {
return typeof LoggerManager !== 'undefined' && LoggerManager !== null && LoggerManager.enable(true);
Copy link
Member

Choose a reason for hiding this comment

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

Same here

packages/rocketchat-lib/lib/core.js
@@ -0,0 +1,8 @@

Copy link
Member

Choose a reason for hiding this comment

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

Where is the .coffee file?

packages/rocketchat-lib/server/startup/settings.js
@@ -0,0 +1,1006 @@
RocketChat.settings.add('uniqueID', process.env.DEPLOYMENT_ID || Random.id(), {
Copy link
Member

Choose a reason for hiding this comment

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

Can you keep the comments?

packages/rocketchat-lib/server/startup/settings.js Outdated
hidden: true
});

RocketChat.settings.addGroup('Accounts', function() {
Copy link
Member

Choose a reason for hiding this comment

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

Can you keep the comments?

packages/rocketchat-lib/server/startup/settingsOnLoadCdnPrefix.js Outdated
/* globals WebAppInternals*/
RocketChat.settings.onload('CDN_PREFIX', function(key, value) {
if (_.isString(value || false)) {
return typeof WebAppInternals !== 'undefined' && WebAppInternals !== null && WebAppInternals.setBundledJsCssPrefix(value);
Copy link
Member

Choose a reason for hiding this comment

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

Simplify this line

packages/rocketchat-lib/server/startup/settingsOnLoadCdnPrefix.js Outdated

Meteor.startup(function() {
const value = RocketChat.settings.get('CDN_PREFIX');
if (_.isString(value || false)) {
Copy link
Member

Choose a reason for hiding this comment

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

Any string value will be considered true?

Copy link
Member Author

Choose a reason for hiding this comment

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

My wish was testing if the variable is a string or it is not an empty string.

"asda"||false - > "asda"
""||false - > false

because _.isString("") === true

Copy link
Member

Choose a reason for hiding this comment

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

Ok, but that looks strange and will allow spaces. What about if (_.isString(value) && value.trim()) just to be more clear?

packages/rocketchat-lib/server/startup/settingsOnLoadCdnPrefix.js Outdated
Meteor.startup(function() {
const value = RocketChat.settings.get('CDN_PREFIX');
if (_.isString(value || false)) {
return typeof WebAppInternals !== 'undefined' && WebAppInternals !== null && WebAppInternals.setBundledJsCssPrefix(value);
Copy link
Member

Choose a reason for hiding this comment

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

Simplify this line

rodrigok
rodrigok approved these changes Apr 3, 2017
View reviewed changes
packages/rocketchat-lib/lib/configLogger.js Outdated
RocketChat.settings.get('Log_Package', function(key, value) {
return (typeof LoggerManager !== 'undefined') && LoggerManager !== null && (LoggerManager.showPackage = value);
return log(LoggerManager => LoggerManager.showPackage = value);
Copy link
Member

Choose a reason for hiding this comment

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

Why not LoggerManager && LoggerManager.showPackage = value?

packages/rocketchat-lib/server/startup/settingsOnLoadCdnPrefix.js Outdated
RocketChat.settings.onload('CDN_PREFIX', function(key, value) {
if (_.isString(value || false)) {
return typeof WebAppInternals !== 'undefined' && WebAppInternals !== null && WebAppInternals.setBundledJsCssPrefix(value);
return testWebAppInternals(WebAppInternals => WebAppInternals.setBundledJsCssPrefix(value));
Copy link
Member

Choose a reason for hiding this comment

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

Why not WebAppInternals && WebAppInternals.setBundledJsCssPrefix(value)?

@rodrigok rodrigok added this to the 0.55.0 milestone Apr 3, 2017
@rodrigok
Copy link
Member

rodrigok commented Apr 4, 2017

@ggazzo Can you fix the conflicts?

@ggazzo
Copy link
Member Author

ggazzo commented Apr 4, 2017

yes, I can.

@engelgabriel engelgabriel merged commit 413ac7a into RocketChat:develop Apr 4, 2017
@ggazzo ggazzo deleted the rocketchat-lib branch April 4, 2017 18:23
@sampaiodiego sampaiodiego mentioned this pull request Apr 4, 2017
Merged
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Reviewers

@rodrigok rodrigok rodrigok approved these changes

Assignees
No one assigned
Labels
None yet
Projects
None yet
Milestone
0.55.0
Development

Successfully merging this pull request may close these issues.
3 participants
@ggazzo @rodrigok @engelgabriel