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

Multiple changes #101

Merged
merged 8 commits into from
Dec 5, 2019
Merged

Multiple changes #101

merged 8 commits into from
Dec 5, 2019

Conversation

raddatzk
Copy link
Contributor

  • add the missing GetCompositeSchedule feature
  • add some missing create(default)Request/Confirmation methods
  • mark the ZeroArgsConstructor as deprecated in favor of the new Constructor with all required arguments
  • Replace Calendar with ZonedDateTime (BREAKING CHANGE in API)
  • add a missing dependency for SOAP (didn't want to compile without 🤔

@TVolden
Copy link
Member

TVolden commented Nov 26, 2019

Hi @kevinraddatz,

Thanks for your PR, it's a big one! 😃

It's going to take me some time to review, but I'll get to it as soon as I have time.

@TVolden
Copy link
Member

TVolden commented Nov 27, 2019

Sorry for the slow pace. It's a busy week for me, but I'll probably have time monday.

@raddatzk
Copy link
Contributor Author

No problem, take your time :)

features.add(new ClearChargingProfileFeature(this));
features.add(new ClearChargingProfileFeature(null));
features.add(new GetCompositeScheduleFeature(null));
features.add(new SetChargingProfileFeature(null));
Copy link
Member

Choose a reason for hiding this comment

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

Why null?

features.add(new TriggerMessageFeature(this));
}
features = new HashSet<>();
features.add(new TriggerMessageFeature(null));
Copy link
Member

Choose a reason for hiding this comment

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

Why null?

features.add(new ReserveNowFeature(this));
features.add(new CancelReservationFeature(this));
features.add(new ReserveNowFeature(null));
features.add(new CancelReservationFeature(null));
Copy link
Member

Choose a reason for hiding this comment

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

Why null?

@raddatzk
Copy link
Contributor Author

raddatzk commented Dec 4, 2019

I set them to null as it seemed like "this" is only required on features that need to be handled by an event handler. The ReserveNow feature for example does not need to be handled on the server so I thought it can be null.
The ServerCoreFeature already had some features set to null, so I thought to continue with it

@TVolden
Copy link
Member

TVolden commented Dec 4, 2019

I get an error when I try to build and test it with maven. Have you tried that?
I'm going to try, to get the CI to work again, so we can get it's report.

@TVolden
Copy link
Member

TVolden commented Dec 4, 2019

Check it out: https://travis-ci.org/ChargeTimeEU/Java-OCA-OCPP/jobs/619203910

unpackPayload_aCalendarPayload_returnsTestModelWithACalendar(eu.chargetime.ocpp.test.JSONCommunicatorTest): Text '2016.03.28T07:16:11.988Z' could not be parsed at index 4

Copy link
Member

@TVolden TVolden left a comment

Choose a reason for hiding this comment

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

Everything looks good.

@TVolden
Copy link
Member

TVolden commented Dec 4, 2019

Got Travis CI working again (wierd gradle permission problem). You probably have to merge in my changes to .travis.yml to make it pass.

@raddatzk
Copy link
Contributor Author

raddatzk commented Dec 5, 2019

Seemd like somehow I missed this test 🤔

@coveralls
Copy link

coveralls commented Dec 5, 2019

Coverage Status

Coverage increased (+1.3%) to 44.751% when pulling d059627 on kevinraddatz:master into 487f960 on ChargeTimeEU:master.

@codecov-io
Copy link

codecov-io commented Dec 5, 2019

Codecov Report

Merging #101 into master will increase coverage by 1.2%.
The diff coverage is 52.67%.

@@             Coverage Diff             @@
##             master     #101     +/-   ##
===========================================
+ Coverage     42.23%   43.44%   +1.2%     
- Complexity      884      923     +39     
===========================================
  Files           214      220      +6     
  Lines          4217     4399    +182     
  Branches        434      436      +2     
===========================================
+ Hits           1781     1911    +130     
- Misses         2329     2374     +45     
- Partials        107      114      +7

@TVolden TVolden merged commit e8ef724 into ChargeTimeEU:master Dec 5, 2019
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.

5 participants