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

Fix for issue #2056 #2058

Merged
merged 1 commit into from
Feb 12, 2022
Merged

Fix for issue #2056 #2058

merged 1 commit into from
Feb 12, 2022

Conversation

BBE78
Copy link
Contributor

@BBE78 BBE78 commented Feb 4, 2022

Added .editorconfig file to customize environment
Updated .eslintrc config file to match rules

@wbt
Copy link
Contributor

wbt commented Feb 4, 2022

Thanks! This looks fine to me; I'll give it some time to see if any other maintainers might have an opinion on this.

@DABH
Copy link
Contributor

DABH commented Feb 5, 2022

Did adding these editor settings not change any files? Was the intent to add editor settings that matched what was already in the repo? If so, this is fine with me, no strong feelings either way.

@BBE78
Copy link
Contributor Author

BBE78 commented Feb 5, 2022

Did adding these editor settings not change any files? Was the intent to add editor settings that matched what was already in the repo? If so, this is fine with me, no strong feelings either way.

Yes, I adapt the config according to existing files (Windows EOL, 2 spaces indent, etc.)

Copy link
Member

@maverick1872 maverick1872 left a comment

Choose a reason for hiding this comment

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

No strong feelings here so long as we're consistent.

@BBE78
Copy link
Contributor Author

BBE78 commented Feb 9, 2022

No strong feelings here so long as we're consistent.

Great!

@maverick1872
Copy link
Member

Fixes #2056

I'll merge this in a couple days if no one else chimes in on this.

@maverick1872 maverick1872 linked an issue Feb 11, 2022 that may be closed by this pull request
@maverick1872 maverick1872 merged commit 30d260d into winstonjs:master Feb 12, 2022
@DABH
Copy link
Contributor

DABH commented Feb 12, 2022

It seems like we didn't merge master into this or something before merging this PR. Master is failing all CI checks due to weird line endings which are coming from the updated settings here. npm run lint should pass. @BBE78 Can you please investigate this and perhaps revert and parts of your PR (e.g. related to line endings) that are causing all the CI failures? Thanks!

@BBE78
Copy link
Contributor Author

BBE78 commented Feb 12, 2022

@DABH I will have a look, seems to be bad EOL in my modified files... 🤔

@BBE78
Copy link
Contributor Author

BBE78 commented Feb 12, 2022

If you're talking about this job, it looks strange.
When I run the npm run lint script, it says:

> winston@3.3.3 lint
> populist lib/*.js lib/winston/*.js lib/winston/**/*.js


C:\Users\Benoît\Documents\GitHub\winston\lib\winston\logger.js
   81:12  warning  Method 'configure' has a complexity of 17                               complexity
   81:12  warning  Method 'configure' has too many statements (20). Maximum allowed is 15  max-statements
  201:6   warning  Method 'log' has a complexity of 12                                     complexity
  201:6   warning  Method 'log' has too many statements (26). Maximum allowed is 15        max-statements

C:\Users\Benoît\Documents\GitHub\winston\lib\winston\tail-file.js
  63:57  warning  Arrow function has a complexity of 13                               complexity
  63:57  warning  Arrow function has too many statements (30). Maximum allowed is 15  max-statements

C:\Users\Benoît\Documents\GitHub\winston\lib\winston\transports\console.js
  44:6  warning  Method 'log' has too many statements (19). Maximum allowed is 15  max-statements

C:\Users\Benoît\Documents\GitHub\winston\lib\winston\transports\file.js
  263:18  warning  'buff' is already declared in the upper scope     no-shadow
  324:29  warning  'options' is already declared in the upper scope  no-shadow

✖ 9 problems (0 errors, 9 warnings)

But the job has been executed on an Ubuntu OS, and if I remember every *nix OS doesn't like CRLF ending lines...
So, to fix this issue, I'will downgrade the CRLF ending lines to a warn (instead of error) in the ESLint config.

@BBE78
Copy link
Contributor Author

BBE78 commented Feb 12, 2022

Fix proposed in Pull Request #2070

@maverick1872
Copy link
Member

Hmm this is interesting as all the checks on the branch passed without issue. Since I merged this I'll also do some investigation.

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.

[Feature Request]: Developper environment improvment
4 participants