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

Misc. fixes to make packaging easier #222

Open
wants to merge 9 commits into
base: dev
Choose a base branch
from

Conversation

Noodlez1232
Copy link

This includes various miscellaneous fixes and changes as described in the issue #221. My recommendation is to not squash this pull request on merging, since the commits are detailed in every change that is done.

Before, both structs were being polluted with the environment variables.
The way it's done now, it is no longer poluted and (in my opinion) is a
bit cleaner as well.
@Noodlez1232 Noodlez1232 marked this pull request as ready for review October 29, 2024 21:06
All paths can now be set in the arguments, and are all derived from the
workPath (by default: ./db). This *should* require no change from the
outside users as well, a transparent change.
Copy link
Owner

@Inrixia Inrixia left a comment

Choose a reason for hiding this comment

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

Linting and Builds are failing.
You can run pnpm run sanity to do a lint and build check.
See my comments for everything else.

src/lib/Attachment.ts Outdated Show resolved Hide resolved
src/lib/defaults.ts Outdated Show resolved Hide resolved
src/lib/helpers/fetchFFMPEG.ts Show resolved Hide resolved
src/lib/helpers/fetchFFMPEG.ts Outdated Show resolved Hide resolved
src/lib/helpers/fetchFFMPEG.ts Outdated Show resolved Hide resolved
src/lib/helpers/index.ts Outdated Show resolved Hide resolved
src/lib/helpers/index.ts Outdated Show resolved Hide resolved
src/lib/Video.ts Outdated Show resolved Hide resolved
@Inrixia
Copy link
Owner

Inrixia commented Nov 1, 2024

Closing due to inactivity. Feel free to re-open if you want to :)

@Inrixia Inrixia closed this Nov 1, 2024
Including the sentinel was confusing postject. That's been moved to a
different file outside the package.json.

postject has also been added to the package.json, since it was being
used, but was not included in the lock. Mostly for completeness.
@Noodlez1232
Copy link
Author

Hi, sorry for the lack of response. I've been busy with uni. I don't have permission to re-open. I've worked on some of the problems you've mentioned, though, and responded to some of the resolutions you've given (including some I disagree with).

@Noodlez1232
Copy link
Author

Actually I'll open this under a new pull request

This moves everything back into the db directory, and only allows the
FFMPEG path to be changed. This also moves around some logic, and
removes unnecessary blobs out of the distribution.
@Inrixia Inrixia reopened this Nov 7, 2024
@Inrixia
Copy link
Owner

Inrixia commented Nov 7, 2024

I am going to reopen this and close #225 to keep this discussion in one place.

package.json Outdated Show resolved Hide resolved
src/lib/Attachment.ts Show resolved Hide resolved
src/lib/helpers/index.ts Show resolved Hide resolved
@Noodlez1232
Copy link
Author

I'd like to also migrate over the question of the settings.json from the other pull request. I was just wondering thoughts on that.

@Inrixia
Copy link
Owner

Inrixia commented Nov 8, 2024

I'd like to also migrate over the question of the settings.json from the other pull request.

Could you copy paste it here thx.

src/lib/helpers/index.ts Show resolved Hide resolved
@Noodlez1232
Copy link
Author

I'd like to start discussion for the separation of settings.json as well.
Since settings.json includes information that could potentially be moved somewhere else (e.g. /etc), it may make sense to add an argument to move its path. The other states (attachments.json, cookies.json, etc.) can still be kept in ./db, since I agree this makes much more sense, but settings.json much like the path of ffmpeg can change based on the distro. (e.g. NixOS allowing setting of settings.json using its own language.)

@Inrixia
Copy link
Owner

Inrixia commented Nov 8, 2024

Re settings.json path I agree, and it's a open issue in #177

I remember there being a few issues with a chicken and egg situation though due to how settings are applied but it's not unworkable

@Noodlez1232
Copy link
Author

Unfortunately I'll have to continue this tomorrow as I'm running out of time for the day. Thanks for being so active!

@Inrixia
Copy link
Owner

Inrixia commented Nov 8, 2024

No problem and no rush :)

Thanks for contributing

@Noodlez1232
Copy link
Author

I'll get the settings path changed and push tomorrow at some point. I've got an idea on how to accomplish that.

@Inrixia
Copy link
Owner

Inrixia commented Nov 17, 2024

Lemme know when this is ready for review

@Noodlez1232
Copy link
Author

Oops my bad yeah It's ready for review I'll mark it as such

@Noodlez1232 Noodlez1232 requested a review from Inrixia November 17, 2024 22:41
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.

2 participants