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

Rename grist-electron to grist-desktop #38

Merged
merged 10 commits into from
Jun 20, 2024
Merged

Conversation

SleepyLeslie
Copy link
Collaborator

Successor of #36 with changes related to the config file reverted to keep backwards compatibility.
This includes some miscellaneous changes:

  • Linting
  • Code structure adjustment and cleanup
  • More info about configuration in the README
  • Package metadata correction
  • Show Grist Desktop version in addition to the underlying Core version in --version

@SleepyLeslie SleepyLeslie force-pushed the sleepyleslie/rename branch 2 times, most recently from 237136c to f4ac13e Compare June 19, 2024 03:54
Copy link
Member

@paulfitz paulfitz left a comment

Choose a reason for hiding this comment

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

Thanks @SleepyLeslie ! Looks broadly reasonable. One case of code duplication I'd like to avoid if possible, I suggest something.



/**
* Copied from grist-core, since it is unsafe to import core code at this point.
Copy link
Member

Choose a reason for hiding this comment

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

Oh wow, bluebird, this code must be old. What is the unsafety issue leading to this need for code duplication? Is it the TYPEORM_DATABASE issue? If so, how do you feel about factoring the method you need in grist-core out to a separate file that doesn't import anything awkward? It will be awkward in future to make sure bugfixes/improvements to this method end up going to the two copies.

Copy link
Collaborator Author

Choose a reason for hiding this comment

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

Thanks for bringing this again to my attention! A closer look suggests that the TYPEORM_DATABASE issue is an urban myth. I remember testing importing dbUtils before setting up TYPEORM_DATABASE and it didn't work, but it likely failed for some other reasons as I couldn't reproduce the failure now (and on the base commit as well). I just did more tests and as a conclusion it could be safely dismissed.

Will remove related hacks and comments. My bad for not catching it early on.

Copy link
Collaborator Author

Choose a reason for hiding this comment

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

Also, I read the dbUtils code, and logically believe that importing it is not supposed to have side effects.

ext/app/electron/main.ts Show resolved Hide resolved
Copy link
Member

@paulfitz paulfitz left a comment

Choose a reason for hiding this comment

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

Thanks for the clean-up! What role does test.db play?

ext/app/electron/main.ts Outdated Show resolved Hide resolved
@SleepyLeslie
Copy link
Collaborator Author

What role does test.db play?

Nothing. It slipped into my commit. Removed.

Copy link
Member

@paulfitz paulfitz left a comment

Choose a reason for hiding this comment

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

Thanks @SleepyLeslie !

@SleepyLeslie SleepyLeslie merged commit 79c356b into main Jun 20, 2024
1 check passed
@SleepyLeslie SleepyLeslie deleted the sleepyleslie/rename branch June 21, 2024 00:33
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