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

feature: Add language files to debugpaste #3645

Open
wants to merge 7 commits into
base: main
Choose a base branch
from

Conversation

Xaver106
Copy link
Contributor

@Xaver106 Xaver106 commented Jun 1, 2022

Overview

Fixes #3176

Description

This should add all language files to the debugpaste.

This currently doesn't work!

The response is always, that a file list needs to be provided in the request, which is obviously the case. I suspect, that there need to be server-side changes for it to accept those files.

Submitter Checklist

  • Make sure you are opening from a topic branch (/feature/fix/docs/ branch (right side)) and not your main branch.
  • Ensure that the pull request title represents the desired changelog entry.
  • New public fields and methods are annotated with @since TODO.
  • I read and followed the contribution guidelines.

@github-actions github-actions bot added the Feature This PR proposes a new feature label Jun 1, 2022
@Xaver106 Xaver106 marked this pull request as ready for review June 1, 2022 21:57
@Xaver106 Xaver106 requested a review from a team as a code owner June 1, 2022 21:57
Copy link
Member

@SirYwell SirYwell left a comment

Choose a reason for hiding this comment

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

I'd prefer the usage of java nio Files and Path, and with proper null checks rather than catching NPEs.

@Xaver106
Copy link
Contributor Author

Xaver106 commented Jun 2, 2022

@SirYwell What would be the benefit of Files and Path?

@SirYwell
Copy link
Member

SirYwell commented Jun 2, 2022

@SirYwell What would be the benefit of Files and Path?

It's the more modern API, it has better exception handling and less magic null values. In best case, we could get rid of any java.io.File usages altogether, but that's clearly out of scope for this PR.

In this case, the code could be written like

Path langDir = PlotSquared.platform().getDirectory().toPath().resolve("lang");
if (Files.isDirectory(langDir)) {
    Files.list(langDir)
        .filter(Files::isRegularFile)
        .filter(path -> path.getFileName().startsWith("message_") && path.getFileName().toString().endsWith(".json"))
        .forEach(path -> incendoPaster.addFile(path.toFile()));

or something like that (I just wrote it down quickly).

@Xaver106 Xaver106 requested review from SirYwell and NotMyFault June 2, 2022 22:04
@Xaver106
Copy link
Contributor Author

Xaver106 commented Jun 2, 2022

You never stop learning. Thanks for explaining @SirYwell

@NotMyFault NotMyFault dismissed SirYwell’s stale review June 3, 2022 09:16

Changes addressed

Copy link
Member

@NotMyFault NotMyFault left a comment

Choose a reason for hiding this comment

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

The change proposed doesn't work at all:

[P2] Failed to create the debugpaste: request must contain a file list

@SirYwell
Copy link
Member

SirYwell commented Jun 4, 2022

The change proposed doesn't work at all:

That looks like a bug with the IncendoPaster or the web service.

@NotMyFault
Copy link
Member

The change proposed doesn't work at all:

That looks like a bug with the IncendoPaster or the web service.

If that is true, this PR must remain on hold until a mandatory PR has been filed against the Paster repository to address this issue, if that is actually an issue, or implement it as feature.

@NotMyFault NotMyFault added the on-hold This PR is on hold until feedback has been addressed label Jun 4, 2022
@Xaver106
Copy link
Contributor Author

Xaver106 commented Jun 5, 2022

The change proposed doesn't work at all:

That looks like a bug with the IncendoPaster or the web service.

If that is true, this PR must remain on hold until a mandatory PR has been filed against the Paster repository to address this issue, if that is actually an issue, or implement it as feature.

@NotMyFault That is the repository for the Java library. Is the code of the web service accessible?
The error code is not from the Paster but sent from the Server. The files were all added successfully. I listed all files with Paster.listFiles(), which listed all files that were supposed to be added.

@NotMyFault
Copy link
Member

NotMyFault commented Jun 5, 2022

There is https://github.com/IntellectualSites/IncendoPasteViewer I guess?
The web server itself runs on Kvantum: https://github.com/Citymonstret/Kvantum

@NotMyFault
Copy link
Member

@Xaver106
Copy link
Contributor Author

Is the server software accesible? The issue seems to be on the server side.

@NotMyFault
Copy link
Member

Is the server software accesible? The issue seems to be on the server side.

See

The web server itself runs on Kvantum: https://github.com/Citymonstret/Kvantum

@Xaver106
Copy link
Contributor Author

Xaver106 commented Jul 24, 2022

@NotMyFault How is the raw code of the web server relevant? As the server only natively serves static content, you either have another software handling incoming files or Kvantum does more than it states in the Readme.
And I also don't understand why I'm supposed to debug it. I'm missing vital information to even have a hope of fixing this issue. I have no information on how the server is set up. You need to ask the server owner to debug, not me.

@dordsor21 seems to be involved in the server, maybe he can help. Or if it is true, that kvantum is handling both serving and receiving files, then maybe @Citymonstret can help.

I have done what was asked of me. I implemented the change in PlotSquares code. Everything else is the Problem of IntellectualSites.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
Feature This PR proposes a new feature on-hold This PR is on hold until feedback has been addressed
Projects
None yet
Development

Successfully merging this pull request may close these issues.

Include /lang/messages_% in debugpaste
3 participants