Seclude ServerlessError into lib/serverless-error.js #8743
Merged
Conversation
This file contains bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
Codecov Report
@@ Coverage Diff @@
## master #8743 +/- ##
=======================================
Coverage 87.43% 87.43%
=======================================
Files 256 257 +1
Lines 9631 9632 +1
=======================================
+ Hits 8421 8422 +1
Misses 1210 1210
Continue to review full report at Codecov.
|
pgrzesik
approved these changes
Jan 11, 2021
| @@ -14,7 +14,7 @@ const Utils = require('./classes/Utils'); | |||
| const Service = require('./classes/Service'); | |||
| const Variables = require('./classes/Variables'); | |||
| const ConfigSchemaHandler = require('./classes/ConfigSchemaHandler'); | |||
| const ServerlessError = require('./classes/Error').ServerlessError; | |||
| const ServerlessError = require('./serverless-error'); | |||
There was a problem hiding this comment.
I have a general question about naming patterns for utilities/modules like that. It seems like currently we mostly have camelCase where I've also noticed some utils with kebab-case. Which one is preferred?
There was a problem hiding this comment.
Very good question, I forgot to discuss that with you. Situation is as follows:
serverlessproject since beginning practiced camelCase- In Node.js community more common was to practise kebab-case. Afaik to avoid confusion between case-sensitive and case insensitive file systems, e.g. now require'ing
serverless/lib/serverless.jswill work on case insensitive systems but will fail on case sensitive systems (with kabab-case it's less likely someone will mix filename casing in require) - Some other projects in this organization practice kebab-case
- As kebab-case felt to me as more natural and safer choice, at some point I decided to shift
serverlessgradually towards kabab-case (I didn't want to imply rename of all modules to avoid eventual breaking changes - but I think with new major we may simply rename all modules to it, to ensure same convention everywhere)
I don't have very strong opinion on that. What do you think?
There was a problem hiding this comment.
Thanks for detailed explanation, it makes sense to me 👍
pgrzesik
approved these changes
Jan 11, 2021
Sign up for free
to join this conversation on GitHub.
Already have an account?
Sign in to comment
Add this suggestion to a batch that can be applied as a single commit.
This suggestion is invalid because no changes were made to the code.
Suggestions cannot be applied while the pull request is closed.
Suggestions cannot be applied while viewing a subset of changes.
Only one suggestion per line can be applied in a batch.
Add this suggestion to a batch that can be applied as a single commit.
Applying suggestions on deleted lines is not supported.
You must change the existing code in this line in order to create a valid suggestion.
Outdated suggestions cannot be applied.
This suggestion has been applied or marked resolved.
Suggestions cannot be applied from pending reviews.
Suggestions cannot be applied on multi-line comments.
Suggestions cannot be applied while the pull request is queued to merge.
Suggestion cannot be applied right now. Please check back later.
Attributes to: #8364
Secludes
ServerlessErrorfromServerlessinternals implementation.This class should be accessible independently of
Serverless, especially after general process error handling was generalized with #8726