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

Feat/support for networks #309

Merged
merged 24 commits into from
May 13, 2024
Merged

Feat/support for networks #309

merged 24 commits into from
May 13, 2024

Conversation

jungwire
Copy link
Contributor

@jungwire jungwire commented May 2, 2024

Negotiator pull request:

Description:

Adds network entities

Checklist:

Make sure you tick all the boxes bellow if they are true or do not apply:

  • I have performed a self review of my code
  • My code follows Google Java Code style
  • I have made my code as simple as possible
  • I have added unit tests and the code coverage has not decreased
  • I have updated the documentation in all relevant places

Copy link

codecov bot commented May 2, 2024

Codecov Report

Attention: Patch coverage is 55.26316% with 17 lines in your changes are missing coverage. Please review.

Project coverage is 79.39%. Comparing base (c760e7a) to head (cb2a44f).

Files Patch % Lines
.../bbmri_eric/negotiator/database/model/Network.java 52.77% 9 Missing and 8 partials ⚠️
Additional details and impacted files
@@             Coverage Diff              @@
##             master     #309      +/-   ##
============================================
- Coverage     79.71%   79.39%   -0.32%     
- Complexity     1076     1091      +15     
============================================
  Files           146      147       +1     
  Lines          3243     3281      +38     
  Branches        173      179       +6     
============================================
+ Hits           2585     2605      +20     
- Misses          543      552       +9     
- Partials        115      124       +9     
Flag Coverage Δ
unit 79.39% <55.26%> (-0.32%) ⬇️

Flags with carried forward coverage won't be shown. Click here to find out more.

☔ View full report in Codecov by Sentry.
📢 Have feedback on the report? Share it here.

@jungwire jungwire force-pushed the feat/support_for_networks branch from 491d74d to b497f09 Compare May 3, 2024 08:24
@jungwire jungwire marked this pull request as ready for review May 3, 2024 08:39
@jungwire jungwire linked an issue May 3, 2024 that may be closed by this pull request
6 tasks
import jakarta.persistence.NamedEntityGraph;
import jakarta.persistence.OneToMany;
import jakarta.persistence.SequenceGenerator;
import jakarta.persistence.*;
Copy link
Collaborator

@RadovanTomik RadovanTomik May 3, 2024

Choose a reason for hiding this comment

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

Copy link
Collaborator

@RadovanTomik RadovanTomik left a comment

Choose a reason for hiding this comment

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

Please see comments and fix the imports with *. It violates google java format, see https://www.jetbrains.com/help/idea/creating-and-optimizing-imports.html

/** The resources of the network. */
@ManyToMany(
cascade = {CascadeType.PERSIST, CascadeType.MERGE},
fetch = FetchType.EAGER)
Copy link
Collaborator

Choose a reason for hiding this comment

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

I would avoid eager fetching as much as possible since we will have some large objects. Any good reason for including it? A network can have thousands of resources

Comment on lines 75 to 92
public void addNetwork(Network network) {
this.networks.add(network);
if (!network.getResources().contains(this)) {
network.addResource(this);
}
}

public Set<Network> getNetworks() {
if (Objects.isNull(this.networks)) {
return Set.of();
}
return Collections.unmodifiableSet(this.networks);
}

public void removeNetwork(Network network) {
this.networks.remove(network);
if (network.getResources().contains(this)) {
network.removeResource(this);
Copy link
Collaborator

Choose a reason for hiding this comment

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

I do not think it makes sense to have this API on both the Resource and the Network. Wouldn't it be better to just have it in one of them?

@jungwire jungwire merged commit abb755c into master May 13, 2024
15 of 17 checks passed
@jungwire jungwire deleted the feat/support_for_networks branch May 13, 2024 11:06
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.

[FEATURE] Add support for networks
2 participants