Skip to content

Copyright header rule for Kotlin files - #1515 #2077

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

Closed
wants to merge 4 commits into from

Conversation

dector
Copy link
Contributor

@dector dector commented Nov 5, 2019

🚧 Draft

@dector dector force-pushed the 1515-license-header-rule branch from ad0b6b7 to 13227c2 Compare November 5, 2019 18:49
@schalkms schalkms requested a review from arturbosch November 5, 2019 19:21
@dector dector force-pushed the 1515-license-header-rule branch from 13227c2 to 5b59e22 Compare November 9, 2019 19:01
@dector
Copy link
Contributor Author

dector commented Nov 9, 2019

Added location property to config. Do you think it's a way to go or should I explore other solutions?

@arturbosch
Copy link
Member

I like the location idea.
It brings to me the idea that we can use the Config as a "data container", storing/caching additional information before actually performing the run (HashMap).
The hard part is that the additional information step is rule dependant.
So basically we need to have a new method for Rule like initData(config: Config) which runs once before the visiting process of the KtFile's.
For this we need to call initData in the core module in the main thread.
This sounds like a generic way for rules to cache data between KtFile's.

@dector
Copy link
Contributor Author

dector commented Nov 11, 2019

Glad that you've liked that.

This sounds like a generic way for rules to cache data between KtFile's.

Sounds awesome! Will explore it. Probably, I'll not have enough time in next 10 days, but will try to find short time-frames.

@dector
Copy link
Contributor Author

dector commented Feb 9, 2020

Sorry for the delay. 😅

Let me double check your suggestions:

  • Should Config serve a purpose of data cache for rules? I'm not sure that it's a very good idea as it'll bloat responsibilities for the class. Besides that what should be do with other config implementations (CompositeConfig and Rule itself as it's extending ConfigAware that is extending Config)?
  • Do you mean that rule.initData() should be invoked somewhere in Detektor.run() (or runSync/runAsync). If I get it right, we'll need to build rule sets once more for this. Is that a lightweight operation?

Thanks!

@arturbosch arturbosch added this to the 1.7.0 milestone Feb 26, 2020
@schalkms
Copy link
Member

schalkms commented Mar 6, 2020

Sorry for the late answer @dector
We'd really like to merge this rule because it provides good value to detekt.

@arturbosch
Copy link
Member

I've rebased on top of master and implemented a way to share the cache license header for this rule.
However I can't push my changes to this PR for whatever reasons...

Normally I check out the fork of a contributor, do some changes and can push them back to this branch. Maybe this time I'm not allowed to do this because it is a draft or something?

@BraisGabin
Copy link
Member

That's a permission that the author of the PR can give when the PR is created

@arturbosch
Copy link
Member

@dector please allow me to push to this branch :)

@dector
Copy link
Contributor Author

dector commented Mar 10, 2020

@arturbosch Hm.. "Edits from maintainers" are enabled for this PR. Let me undraft it.

@dector dector marked this pull request as ready for review March 10, 2020 07:00
@arturbosch
Copy link
Member

arturbosch commented Mar 10, 2020

Unfortunately I couldn't push into this PR and created a new PR with the caching changes on top of your commits ... :(

@arturbosch arturbosch closed this Mar 10, 2020
@arturbosch arturbosch mentioned this pull request Mar 10, 2020
@arturbosch arturbosch changed the title #1515. Copyright header rule for Kotlin files. Copyright header rule for Kotlin files - #1515 Mar 22, 2020
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.

4 participants