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

Qual isolate ExpeditionLigne class to separate file #30877

Merged
merged 10 commits into from
Sep 12, 2024

Conversation

W1W1-M
Copy link
Contributor

@W1W1-M W1W1-M commented Sep 6, 2024

Qual isolate ExpeditionLigne class to separate file

Like #30784 this moves the line class to a separate file.

In this PR, the Expedition class file length is reduced by 18% about 600 lines, from 3200 to 2600 lines.

@W1W1-M W1W1-M changed the title Qual isolate ExpeditionLgine class to separate file Qual isolate ExpeditionLigne class to separate file Sep 6, 2024
@W1W1-M
Copy link
Contributor Author

W1W1-M commented Sep 6, 2024

@mdeweerd any idea for fixing this Phan type warning ? or does it need some sort of exception ? Thanks

@mdeweerd
Copy link
Contributor

mdeweerd commented Sep 6, 2024

any idea for fixing this Phan type warning ? or does it need some sort of exception ? Thanks

I think we can agree that this is a bug in the code.

$this->errors[] is string[] and we are appending a string[] item to it, which makes it a array<string|string[]> .

So these solutions exist:

  • Append the list of error to the pre-existing list $this->errors = array_merge($this->errors, $lot->errors); (possibly ensuring first that $this->errors is indeed an array).
  • Change the type for $this->errors to array<string|string[]> ;
  • Substitute $this->errors: $this->errors = $lot->errors.

I suppose that the first option is the correct one.

@W1W1-M
Copy link
Contributor Author

W1W1-M commented Sep 9, 2024

any idea for fixing this Phan type warning ? or does it need some sort of exception ? Thanks

I think we can agree that this is a bug in the code.

$this->errors[] is string[] and we are appending a string[] item to it, which makes it a array<string|string[]> .

So these solutions exist:

  • Append the list of error to the pre-existing list $this->errors = array_merge($this->errors, $lot->errors); (possibly ensuring first that $this->errors is indeed an array).
  • Change the type for $this->errors to array<string|string[]> ;
  • Substitute $this->errors: $this->errors = $lot->errors.

I suppose that the first option is the correct one.

Thanks for help. I wasn't seeing it was the errors array 😅 Would there be any way to improve the Phan output on Github ? It seems like there are special chars not being formatted correctly, like Assigning ($lot-&gt;errors as a field) or am I misreading this ?
image

@W1W1-M W1W1-M marked this pull request as ready for review September 9, 2024 09:15
@mdeweerd
Copy link
Contributor

mdeweerd commented Sep 9, 2024

Concerning the representation of the messages: I think an option needs to be added to https://github.com/mdeweerd/logtocheckstyle to decode the url encoded messages or to not encode them (or maybe automatic with possibility to force it).

@eldy eldy added the PR to fix - See feedback in comments PR needs to be fixed to be integrated (some comments should describes the fix to do) label Sep 9, 2024
@mdeweerd
Copy link
Contributor

@W1W1-M I examined the possibility to parse the url encoded message, but it is cs2pr that is used to generate the github annotations directly from the checkstyle compatible xml generated by phan.
I thought it was using logToCheckStyle to do this from a textual output.
So either phan or cs2pr would need to evolve.

@W1W1-M
Copy link
Contributor Author

W1W1-M commented Sep 11, 2024

@W1W1-M I examined the possibility to parse the url encoded message, but it is cs2pr that is used to generate the github annotations directly from the checkstyle compatible xml generated by phan. I thought it was using logToCheckStyle to do this from a textual output. So either phan or cs2pr would need to evolve.

Thanks for looking into this ! It would be nicer for dev experience. Not sure how much work is required but it would be good to get this done at some point.

@eldy eldy merged commit 7600157 into Dolibarr:develop Sep 12, 2024
7 checks passed
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
PR to fix - See feedback in comments PR needs to be fixed to be integrated (some comments should describes the fix to do)
Projects
None yet
Development

Successfully merging this pull request may close these issues.

3 participants