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

[Feature] Implementation of the public discord statuspage api #55

Open

Almighty-Alpaca wants to merge 15 commits into master from feature/statuspage

Member

Almighty-Alpaca commented Feb 15, 2018 •

edited

Loading

which can be found here. This currently does not include the uptime tracker or incident history (more than last 50 incidents) as they are not part of the public api. Probably also still needs a bit more documentation.

Almighty-Alpaca added 6 commits

February 13, 2018 15:58


          Implemented public statuspage API

e548b9e


          Added tests, removed the need of a ThreadPool and fixed parsing of

e31257c

OffsetDateTimes


          Hid the CompletableFuture class behing an interface

cd2c50d

to deny access to internal methods.


          Made a little gradle thing more beautiful

7c55975


          Changed enum implementation

f86ce38


          Reordered methods

3c4b5a9

Almighty-Alpaca added the Enhancement label

Almighty-Alpaca self-assigned this

Almighty-Alpaca requested review from jagrosh and Shengaero

February 21, 2018 15:54

Shengaero requested changes

View reviewed changes

Collaborator

Shengaero left a comment

This PR needs work.
I saw no documentation, very inconsistent code-style, improper and overuse of the javax.annotation API, and other things that preclude it from being merged.

statuspage/src/main/java/com/jagrosh/jdautilities/statuspage/Statuspage.java Outdated

+                  @Nonnull
+                  protected static final String URL_SCHEDULED_MAINTENANCES_ACTIVE = URL_API_BASE + "scheduled-maintenances/active.json";
+                  @Nonnull
+                  protected static final String URL_SCHEDULED_MAINTENANCES_UPCOMING = URL_API_BASE + "scheduled-maintenances/upcoming.json";

Collaborator

Shengaero Feb 21, 2018

For the scheduled-maintenances endpoints and organization's sake, we should have a base url

    @Nonnull
    protected static final String URL_SCHEDULED_MAINTENANCES_BASE = URL_API_BASE + "scheduled-maintenances";
    @Nonnull
    protected static final String URL_SCHEDULED_MAINTENANCES_ALL = URL_SCHEDULED_MAINTENANCES_BASE + ".json";
    @Nonnull
    protected static final String URL_SCHEDULED_MAINTENANCES_ACTIVE = URL_SCHEDULED_MAINTENANCES_BASE + "/active.json";
    @Nonnull
    protected static final String URL_SCHEDULED_MAINTENANCES_UPCOMING = URL_SCHEDULED_MAINTENANCES_BASE + "/upcoming.json";

The same might be worth doing for incidents although I'll leave that up to you.

statuspage/src/main/java/com/jagrosh/jdautilities/statuspage/Statuspage.java Outdated

+                      @Nonnull
+                      final JSONObject statusObject = object.getJSONObject("status");
+                      @Nonnull
+                      final Status status = createStatus(statusObject);

Collaborator

Shengaero Feb 21, 2018

I don't see a reason to have @Nonnull, @Nullable, and similar kinds of annotations in method bodies.
I won't make you remove any of the method, class, and field annotations, but I would be hesitant on adding nullable annotations in the future as I have been considering adding them to the library myself, but only if and when feature/nullability is merged.

statuspage/src/main/java/com/jagrosh/jdautilities/statuspage/Statuspage.java Outdated

+                      @Nonnull
+                      final Page page = createPage(pageObject);
+                      return new Incidents(page, incidents);

Collaborator

Shengaero Feb 21, 2018

See my comment on @Nonnull and @Nullable in method bodies

statuspage/src/main/java/com/jagrosh/jdautilities/statuspage/Statuspage.java Outdated

+                      @Nonnull
+                      final List<ScheduledMaintenance> scheduledMaintenances = new ArrayList<>(scheduledMaintenancesArray.length());
+                      for (int i = 0; i < scheduledMaintenancesArray.length(); i++)
+                          scheduledMaintenances.add(createScheduledMaintenance(scheduledMaintenancesArray.getJSONObject(i)));

Collaborator

Shengaero Feb 21, 2018

See my comment on @Nonnull and @Nullable in method bodies

statuspage/src/main/java/com/jagrosh/jdautilities/statuspage/Statuspage.java Outdated

+                      @Nonnull
+                      final JSONObject statusObject = object.getJSONObject("status");
+                      @Nonnull
+                      final Status status = createStatus(statusObject);

Collaborator

Shengaero Feb 21, 2018

See my comment on @Nonnull and @Nullable in method bodies

statuspage/src/main/java/com/jagrosh/jdautilities/statuspage/endpoints/Incidents.java Outdated

+                  @Override
+                  @Nonnull
+                  public List<Incident> subList(int fromIndex, int toIndex) {return incidents.subList(fromIndex, toIndex);}

Collaborator

Shengaero Feb 21, 2018

This formatting seems fairly... odd?

...spage/src/main/java/com/jagrosh/jdautilities/statuspage/endpoints/ScheduledMaintenances.java Outdated

+                  @Override
+                  @Nonnull
+                  public List<ScheduledMaintenance> subList(int fromIndex, int toIndex) {return ScheduledMaintenances.subList(fromIndex, toIndex);}

Collaborator

Shengaero Feb 21, 2018

This formatting seems fairly... odd?

...spage/src/main/java/com/jagrosh/jdautilities/statuspage/endpoints/ScheduledMaintenances.java Outdated

+                  @Nonnull
+                  protected final Page page;
+                  @Nonnull
+                  protected final List<ScheduledMaintenance> ScheduledMaintenances;

Collaborator

Shengaero Feb 21, 2018

What is this formatting...?

Member Author

Almighty-Alpaca Feb 21, 2018

I used a json2pojo generator, probably just didn't saw what it has done here.

...spage/src/main/java/com/jagrosh/jdautilities/statuspage/endpoints/ScheduledMaintenances.java Outdated

+                  public <T> T[] toArray(@Nonnull T[] a) {return ScheduledMaintenances.toArray(a);}
+                  @Override
+                  public boolean add(ScheduledMaintenance ScheduledMaintenance) {return ScheduledMaintenances.add(ScheduledMaintenance);}

Collaborator

Shengaero Feb 21, 2018

This class is marked as @Immutable yet we are allowing modification of the underlying list?
This should throw UnsupportedOperationException or the List interface should not be implemented at all.
(This goes for all other methods that might modify this object)

statuspage/src/test/java/com/jagrosh/jdautilities/statuspage/tests/StatuspageTest.java Outdated

+                  {
+                      statuspage.getSummary().get();
+                  }
+              }

Collaborator

Shengaero Feb 21, 2018

Can we remove this test class?

Member Author

Almighty-Alpaca Feb 21, 2018

What is wrong with tests?

Collaborator

Shengaero commented Feb 21, 2018

This would also be a good time to mention that changes to our build setup should be made to allow certain modules to not be included in the collective dependency (IE: com.jagrosh:jda-utilities:VERSION) because this module is not something most people who compile the collective dependency for their bots would even use.

Shengaero added Proposal Version: 2.2 Feature and removed Version: 2.2 Enhancement labels

Almighty-Alpaca added 3 commits

February 21, 2018 19:28


          Allowing submodules to not be included in parent

7d52a82

and implemented this for the statuspage module.


          Cleanup

8c1a572

- Fixed immutable classes
- Removed nullability annotation abuse
- Removed List interface from classes as yo ucan get the list via a method


          Fixed field type

4aff9ab

Shengaero changed the title ~~Implementation of the public discord statuspage api~~ [Feature] Implementation of the public discord statuspage api

jagrosh force-pushed the master branch from a977f3c to 7f16d4b Compare

May 18, 2018 18:25

Shengaero added 6 commits

May 31, 2018 13:20


          Merge branches 'feature/statuspage' and 'master' of https://github.co…

5f794c7

…m/JDA-Applications/JDA-Utilities into feature/statuspage

# Conflicts:
#	build.gradle


          Merge branches 'feature/statuspage' and 'master' of https://github.co…

6de3d15

…m/JDA-Applications/JDA-Utilities into feature/statuspage

# Conflicts:
#	build.gradle
#	command/build.gradle
#	doc/build.gradle
#	examples/build.gradle
#	menu/build.gradle
#	settings.gradle


          fixed statuspage build

4e8cca7


          Merge branches 'feature/statuspage' and 'master' of https://github.co…

fc81c2d

…m/JDA-Applications/JDA-Utilities into feature/statuspage


          done with that!

61dbd2e


          Renamed Statuspage to StatusPage

e932eac

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Labels

Feature Proposal