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

Fixed window title update & occasional sqlite error for packages #560

Merged
merged 1 commit into from
May 1, 2023

Conversation

OmkarPh
Copy link
Collaborator

@OmkarPh OmkarPh commented Apr 19, 2023

Fixes #549

Signed-off-by: Omkar Phansopkar <omkarphansopkar@gmail.com>
@OmkarPh OmkarPh changed the title Fixed window not found & occasional sqlite error for packages Fixed window title update & occasional sqlite error for packages Apr 19, 2023
Copy link
Member

@AyanSinhaMahapatra AyanSinhaMahapatra left a comment

Choose a reason for hiding this comment

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

@OmkarPh this looks good, merging this. See also comments for your future consideration.

But we need tests running on CI: both GitHub Actions and Azure in linux/mac/windows (And also make sure local builds are working on these platforms too) for a stable release of workbench btw.

// remove the .json (or other) extention of the path.
defaultPath = jsonFilePath.substring(0, jsonFilePath.lastIndexOf('.')) + '.sqlite';
defaultPath =
jsonFilePath.substring(0, jsonFilePath.lastIndexOf(".")) + ".sqlite";
} else {
// FIXME: this is some ugly regex used to get filename with no extension.
// see: https://stackoverflow.com/questions/4250364/how-to-trim-a-file-extension-from-a-string-in-javascript
Copy link
Member

Choose a reason for hiding this comment

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

We need to document the file license when we copy code from elsewhere, and also have a common notice for all files like we have in scancode-toolkit and elsewhere: for example see https://github.com/nexB/scancode-toolkit/blob/develop/src/licensedcode/cache.py#L4

Copy link
Collaborator Author

Choose a reason for hiding this comment

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

all stackoverflow code is licensed under CC BY-SA right
so, exactly how should we show this in this file

Copy link
Member

Choose a reason for hiding this comment

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

This one specifically under https://creativecommons.org/licenses/by-sa/3.0/ yes. We should probably put a line and reference to the text, but I'm not sure of the effect of a sharealike license on the whole file/repo license. @pombredanne @DennisClark what would be recommended here?

Copy link
Member

Choose a reason for hiding this comment

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

These regex are too small to be relevant copyright-wise IMHO... but this code is also arcane, and this is best rewritten with a cleaner and more readable way.

}
mainWindow.webContents.send(IMPORT_REPLY_CHANNEL.JSON, reply);
});
dialog
Copy link
Member

Choose a reason for hiding this comment

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

We could have a default here that we can set, like a specific folder that we save the sqlite file in, to save time. This is a refinement of course, no need to do this in this PR.

Copy link
Collaborator Author

Choose a reason for hiding this comment

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

yeah makes sense, maybe settings kinda option for users to set the default path

@AyanSinhaMahapatra AyanSinhaMahapatra merged commit 1759b67 into v4.0-react-typescript May 1, 2023
@OmkarPh
Copy link
Collaborator Author

OmkarPh commented May 1, 2023

@OmkarPh this looks good, merging this. See also comments for your future consideration.

But we need tests running on CI: both GitHub Actions and Azure in linux/mac/windows (And also make sure local builds are working on these platforms too) for a stable release of workbench btw.

yeah
these tests should only cover the parsers or desktop app itself too ?

@AyanSinhaMahapatra
Copy link
Member

these tests should only cover the parsers or desktop app itself too ?

Ideally a bit of both? That's not too hard to test right? minimal unit tests and end-to-end tests to ensure most of the functions/parsers and views work? And keep adding specific cases based on bug reports (reproducing by adding a test and then fixing). I'm not familiar at all with ts/react testing so food for thought for you, what would be the way forward here

@DennisClark
Copy link
Member

I think that simply saying that some code is licensed under https://creativecommons.org/licenses/by-sa/4.0/ is sufficient and does not have a significant impact on the overall license of the project, which is all open source with the source code freely available already, and SCWB is basically a standalone application.

@AyanSinhaMahapatra AyanSinhaMahapatra deleted the fix/importCrash branch October 25, 2023 18:55
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.

4 participants