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.

Sign up for GitHub

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

Jump to bottom

PLC4X-88: Add Triggering to PLC Scraper #46

Merged
merged 1 commit into from
Mar 7, 2019
Merged

PLC4X-88: Add Triggering to PLC Scraper #46

merged 1 commit into from
Mar 7, 2019

Conversation

timbo2k
Copy link
Contributor

@timbo2k timbo2k commented Mar 5, 2019

  • introduced TriggeredScraper and belonging classes
  • made classes of old scraper deprecated
  • added scheduled and S7-variable trigger (boolean and numeric)
  • java-docs

@timbo2k
Copy link
Contributor Author

timbo2k commented Mar 5, 2019 •
edited
Loading

Hi everybody

I had worked on the triggered scraper as stated on the list.
The pushed branch is the current version to retrieve your opinion ... so handle it as working state.
I will also add relevant tests for the new triggered scraper.
mvn verify went fine except i commented out one test --> ScraperConfigurationBuilderTest maybe @JulianFeinauer can have a look on that, test was written by you

Furthermore i declared the "old" scraper as deprecated because triggered one can supply everything what old scraper could do; only for compatability-reasons

I'm looking forward for your comments ... please do not merge until i added the tests i planned to implement.

Best
Tim

@JulianFeinauer JulianFeinauer changed the title PLC4X-88: PLC4X-88: Add Triggering to PLC Scraper Mar 6, 2019
@JulianFeinauer
Copy link
Contributor

Hey @timbo2k I'm pretty impressed by your work (especially I like the syntax we now have for the triggering modes).
I added a lot of small points which are mostly no-brainers to fix.
If we add some more tests I think we are ready to merge ASAP!.

Very well done 👍

JulianFeinauer
JulianFeinauer reviewed Mar 6, 2019
View reviewed changes
Copy link
Contributor

@JulianFeinauer JulianFeinauer left a comment

Choose a reason for hiding this comment

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

Sorry, here are my comments.

plc4j/utils/scraper/pom.xml Show resolved Hide resolved
plc4j/utils/scraper/src/main/java/org/apache/plc4x/java/scraper/ResultHandler.java Outdated Show resolved Hide resolved
plc4j/utils/scraper/src/main/java/org/apache/plc4x/java/scraper/ScrapeJob.java Outdated Show resolved Hide resolved
plc4j/utils/scraper/src/main/java/org/apache/plc4x/java/scraper/Scraper.java Outdated Show resolved Hide resolved
plc4j/utils/scraper/src/main/java/org/apache/plc4x/java/scraper/Scraper.java Show resolved Hide resolved
...java/org/apache/plc4x/java/scraper/triggeredscraper/triggerhandler/TriggerConfiguration.java Outdated Show resolved Hide resolved
...java/org/apache/plc4x/java/scraper/triggeredscraper/triggerhandler/TriggerConfiguration.java Outdated Show resolved Hide resolved
...java/org/apache/plc4x/java/scraper/triggeredscraper/triggerhandler/TriggerConfiguration.java Show resolved Hide resolved
...java/org/apache/plc4x/java/scraper/triggeredscraper/triggerhandler/TriggerConfiguration.java Show resolved Hide resolved
.../main/java/org/apache/plc4x/java/scraper/triggeredscraper/triggerhandler/TriggerHandler.java Show resolved Hide resolved
@timbo2k
PLC4X-88:
527ae1b 
- introduced TriggeredScraper and belonging classes
- made classes of old scraper deprecated
- added scheduled and S7-variable trigger (boolean and numeric)
- java-docs
- tests for regex pattern of trigger strategy added
- added ToDos
@timbo2k
Copy link
Contributor Author

timbo2k commented Mar 6, 2019

changed most of the issue from @JulianFeinauer
for refactoring and rework to be more generic i created issues PLC4X-89 and PLC4X-90

@JulianFeinauer
Copy link
Contributor

Hey, from my perspective all issues are adressed (either directly or via the new Jira Issues) so I feel fine with this PR.
So feel free to merge your first PR yourself 🎉

@JulianFeinauer JulianFeinauer mentioned this pull request Mar 7, 2019
Merged
@timbo2k
Copy link
Contributor Author

timbo2k commented Mar 7, 2019

I will close discussion and merge PR into master

@timbo2k timbo2k merged commit ee46d30 into apache:develop Mar 7, 2019
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Reviewers

@JulianFeinauer JulianFeinauer JulianFeinauer left review comments

Assignees
No one assigned
Labels
None yet
Projects
None yet
Milestone
No milestone
Development

Successfully merging this pull request may close these issues.

None yet
2 participants
@timbo2k @JulianFeinauer