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

become functional on my local instance #30

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

Conversation

an-dr-eas-k
Copy link

Dear @gergelykalman ,

this PR renders your confluence-markdown-exporter more functional. It addresses the following details that I need to overcome for my demand:

  1. adjust token handling, and use token also for download of attachments (fixes Unable to download attachments (401 unauthorized) + solution #25)
  2. increase the possible number of expected spaces on a instance, the previous limit of 500 was not sufficient for our instance (fixes Support proper paging #28)
  3. dynamic download url to download attachments (Wrong url when downloading attachments #22)
  4. fix an exception and last
  5. increase the number of chars to sanitize

Please take a look and see if it suits your style.

Copy link
Owner

@gergelykalman gergelykalman left a comment

Choose a reason for hiding this comment

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

You removed username auth here, if you ever want this to get merged please introduce it in a way that it doesn't break the code for people who are using usernames and passwords.

Copy link
Owner

@gergelykalman gergelykalman left a comment

Choose a reason for hiding this comment

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

This is wrong:

  • You removed the error message about wrong credentials
  • You changed the default limits
  • The if statement on line 137 is bad, organize that so that __dump_space() is not called from two places, also why is return used here instead of break?

Copy link
Owner

@gergelykalman gergelykalman left a comment

Choose a reason for hiding this comment

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

This is very wrong, ALWAYS print the exception if you ever decide to do a catch-all, and even then it's probably a bad idea.

Copy link
Owner

@gergelykalman gergelykalman left a comment

Choose a reason for hiding this comment

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

Put the error message back, or alternatively create a command line switch that can turn verbose logs off for your specific use-case (but the default should be verbose: ON)

Copy link
Owner

@gergelykalman gergelykalman left a comment

Choose a reason for hiding this comment

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

Restore the original version, we want to keep the filename as close to the original as possible. Since we support unicode by default, this list is pretty worthless as unicode can be used to bypass it. Also \n and other nasty ASCII chars are still permitted, so if you want to process these from a shell script or something, be VERY careful

@gergelykalman
Copy link
Owner

Don't get discouraged by the comments, this is a good first attempt! I just want the code to be usable for every other person who doesn't have your specific use-case.

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.

Support proper paging Unable to download attachments (401 unauthorized) + solution
2 participants