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

Refactor file processors towards universal collector #6085

Merged
merged 11 commits into from
Apr 12, 2021

Conversation

TomasVotruba
Copy link
Member

@TomasVotruba TomasVotruba commented Apr 10, 2021

@TomasVotruba TomasVotruba force-pushed the file-processor-2 branch 2 times, most recently from 0c6fa36 to 1b2798d Compare April 10, 2021 14:58
@TomasVotruba TomasVotruba changed the title file processor 2 Make file processor return string Apr 10, 2021
@TomasVotruba TomasVotruba changed the title Make file processor return string [WIP] Make file processor return string Apr 10, 2021
@TomasVotruba TomasVotruba force-pushed the file-processor-2 branch 10 times, most recently from 74fcd74 to 9a7532a Compare April 10, 2021 23:30
@TomasVotruba TomasVotruba force-pushed the file-processor-2 branch 2 times, most recently from 6301863 to 97bc86e Compare April 11, 2021 10:30
@TomasVotruba TomasVotruba force-pushed the file-processor-2 branch 3 times, most recently from 3cabf7d to a52080c Compare April 11, 2021 13:39
@TomasVotruba TomasVotruba changed the title [WIP] Make file processor return string Refactor file processors towards universal collector Apr 11, 2021
@TomasVotruba TomasVotruba force-pushed the file-processor-2 branch 2 times, most recently from cdac2ba to 0a88070 Compare April 11, 2021 22:12
@TomasVotruba TomasVotruba force-pushed the file-processor-2 branch 4 times, most recently from c3b58f5 to ad7fe54 Compare April 12, 2021 08:46
@TomasVotruba TomasVotruba force-pushed the file-processor-2 branch 2 times, most recently from 39da79d to 23b5724 Compare April 12, 2021 09:00
@TomasVotruba TomasVotruba enabled auto-merge (squash) April 12, 2021 09:13
@lulco
Copy link
Contributor

lulco commented Apr 12, 2021

@TomasVotruba Hi, I merged it into my branch with neon files. Content stops changing :) in my processor it is changed and I update content at the end of processing using $file->changeFileContent($newContent).

I got this diff:

6) app/config.neon

    ---------- begin diff ----------
--- Original
+++ New
    ----------- end diff -----------

So something is wrong :)

@TomasVotruba
Copy link
Member Author

TomasVotruba commented Apr 12, 2021

This should be called in the end of process() method:

$this->fileDiffFileDecorator->decorate([$file]);

It decorates the file with its diff.

@lulco
Copy link
Contributor

lulco commented Apr 12, 2021

In all processors? I didn’t see that in existing processors. I found it in ApplicationFileProcessor::run. It should be enough, right?

@TomasVotruba
Copy link
Member Author

Not sure. I'll try to debug...

@lulco
Copy link
Contributor

lulco commented Apr 12, 2021

I can prepare some failing small application later tonight if you will need

@TomasVotruba
Copy link
Member Author

TomasVotruba commented Apr 12, 2021

So there must be a printer, because every processor has its own - PHP has php-parser printer, JSON has json printer etc.


The best inspiration would be in the smaller one ComposerFileProcessor:

$changeFileContent = $this->composerJsonPrinter->printToString($composerJson);
$file->changeFileContent($changeFileContent);

UPDATED

@TomasVotruba
Copy link
Member Author

can prepare some failing small application later tonight if you will need

Failing CI is always the best 👍

@lulco
Copy link
Contributor

lulco commented Apr 12, 2021

The best inspiration would be in the smaller one ComposerFileProcessor:

OK I don't get it :) printer just creates new content as a string, then method changeFileContent is called and that's all...

How is it possible that it works with composer, but not with NonPhpFileProcessor and NetteDINeonMethodCallRenamer too?

@lulco
Copy link
Contributor

lulco commented Apr 12, 2021

can prepare some failing small application later tonight if you will need

Failing CI is always the best 👍

I'm sure it is :) but this is a test for whole application, because all parts are working standalone, but not together :)

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