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

Various Changes for #1 #812

Closed
wants to merge 3 commits into from
Closed

Conversation

spnettec
Copy link

fix OutOfMemoryError
fix S7 driver
add s7-200
add Timeout-HandlingVarious ChangesExtended syntax of tags to add "encoding" to s7 string fields

fix OutOfMemoryError
fix S7 driver
add s7-200
add Timeout-Handling
Extended syntax of tags to add "encoding" to s7 string fields
@spnettec
Copy link
Author

spnettec commented Feb 17, 2023

This is basic fix. I need to ensure that these basic fixes can be merged before doing further merges

@spnettec
Copy link
Author

By the way, I'm heyoulin. I have two github account

@ottlukas ottlukas requested a review from chrisdutz February 17, 2023 17:10
@chrisdutz
Copy link
Contributor

This is a huge PR to chew on ... and I did have a look when I created the big PR for you ... we'll definitely not take all changes and we'll do some things differently. So I would not like to simply merge all of this. Also can't I review 2500 files. I think I wen't through most of them in the PR that I created, but I'm not going to do it again.

I think we need to manually pull individual changes into individual PRs and over time merge in the stuff we want to see merged.

@chrisdutz
Copy link
Contributor

It's also a great example why it's not that good of an idea to work on a fork and not have the changes pulled back ... I hope I'll have the time to pull some of the changes back this weekend, but I was currently working on getting some EIP driver changes in. Currently a bit much for one person to do ... would be cool, if some of the other comitters could also jump in and have a look at #784 first ... that was the one where I wen't though most changes.

@chrisdutz
Copy link
Contributor

But ... thanks a lot for officially creating a PR ... this IS really highly appreciated. Now all we need to do is get you back and then, if we work a lot closer from then on, this will be a lot better for all :-)

@sruehl
Copy link
Contributor

sruehl commented Feb 21, 2023

can you run a maven clean install and commit that? looks like there is a bunch of formatting which affected a huge amount of files.

@sruehl sruehl marked this pull request as draft February 21, 2023 16:22
@chrisdutz
Copy link
Contributor

I intentionally didn't pull the generated code changes into that other PR I created as it was swamping the usefull bits of the PR

@sruehl
Copy link
Contributor

sruehl commented Feb 21, 2023

just to clear up that comment was directed at @spnettec

@spnettec
Copy link
Author

It is not huge PR. I run maven clean install to make sure everything compiles fine. You can ignore generated code changes

@chrisdutz
Copy link
Contributor

Yeah ... but we won't merge this PR ... as it contains many changes that we would have to discuss first and currently would probably solve differently (Like the one with the additional encoding for string tags) .... we're currently disucssing a similar extension on the dev-list, wehre discussions like this belong for an apache project.

@spnettec
Copy link
Author

I saw the discuss. I know the problem. But i think we should resolve user urgent problem. We can change that at next version. For me the additional encoding for string tags is important now.

@chrisdutz
Copy link
Contributor

Well the thing is: I'm currently working most of my time, cleaning up things. So I will definitely not approve an approach like this. If the others do, they will have to do the house-keeping for it. I know how stuff like this happened in the past ... stuff was merged with the promise to clean up later, but then nothing happened and I'm currently working on cleaning it up.

This is not the way Open-Source projects (at Apache work) ... we don't like huge code-drops like this. Even if the amount of code isn't that huge, but the number of changes are.

@ottlukas
Copy link
Contributor

ottlukas commented Feb 22, 2023

I saw the discuss. I know the problem. But i think we should resolve user urgent problem. We can change that at next version. For me the additional encoding for string tags is important now.

There is a balance between you specific urgent problem and a stable plc4x framework / open source in a foundation like the ASF. If the refactoring of code is extensive and can not forseen it is against the basic principle of Community over Code. If you can convince the community on the mailing list to accept your PR then it would work. To be realistic.... the changes are too big and it would be hard to get a majority with this amount of changes. So we like to pull in you really nice changes in a slower pace and decouple it by looking first and the non-breaking elements and for the string topic you already saw the dicussion on the mailing list. I hope that helps you to understand a little better as we are aiming to have a community around the code and a stable framework where developers are comfortable on supporting and evolving PLC4X. So for me I can not support this PR for that reasons.

@spnettec
Copy link
Author

I will close this PR

@spnettec spnettec closed this Feb 22, 2023
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