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

adding keycloak integration to app-builder #548

Merged
merged 5 commits into from
Jul 16, 2019
Merged

Conversation

sergiofilhowz
Copy link
Contributor

No description provided.

@entando-service
Copy link
Contributor

Something went wrong. Investigate!

@entando-service
Copy link
Contributor

All is well

@entando-service
Copy link
Contributor

All is well

@entando-service
Copy link
Contributor

All is well

},
loader: require.resolve('eslint-loader'),
}];

Copy link
Contributor

Choose a reason for hiding this comment

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

why are we giving the option to disable the linter?

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Hi Nicola, let me give you an example:

While I was developing I was receiving some weird errors on the Context provider (that was because of the version of react-dom), The error logs couldn't help a bit, so I had to filter what was causing so I had to remove some elements one by one to identify what element was causing this. Every time I removed one element, I had to work on on unused variables, unused imports, and a lot of things on the eslint, all of them unnecessary to the work I was doing, because none of the changes I wasn't going to commit. I can say that I lost (literally lost) 3 hours in total doing fixes on the code to be eslint compliant. So, with an option to disable temporarily this linter will help on those situations.

I literally lost 3 hours because all the changes I did to identify the problem resulted on a single line commit on package.json

Copy link
Contributor

Choose a reason for hiding this comment

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

hi Sergio, you could have just added the disable eslint comment in the file.

/* eslint-disable */

alert('foo');

/* eslint-enable */

this is totally unnecessary since it just disable eslint globally on the whole project which should not be needed

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Sometimes I had to remove a large block and makes several lines unused. Anyway, I'll do what you want since you are saying it's "totally unnecessary". I'm going to fix this.

@entando-service
Copy link
Contributor

All is well

@nicpuddu nicpuddu merged commit 060da55 into master Jul 16, 2019
@nicpuddu nicpuddu deleted the EN6-32-keycloak branch July 16, 2019 13:07
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.

3 participants