Skip to content

Drop redundant columns from files and folders relations #6477

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

Closed
wants to merge 11 commits into from

Conversation

hvitved
Copy link
Contributor

@hvitved hvitved commented Aug 12, 2021

No description provided.

@hvitved hvitved force-pushed the files-folders-drop-columns branch from 8ffea3e to c4812ab Compare August 12, 2021 12:37
@hvitved hvitved changed the title Files folders drop columns Drop redundant columns from files and folders relations Aug 12, 2021
@hvitved hvitved added the depends on internal PR This PR should only be merged in sync with an internal Semmle PR label Aug 12, 2021
@hvitved hvitved force-pushed the files-folders-drop-columns branch 4 times, most recently from 002975e to 34588d9 Compare August 16, 2021 08:10
@hvitved hvitved marked this pull request as ready for review August 16, 2021 10:39
@hvitved hvitved requested review from a team as code owners August 16, 2021 10:39
@hvitved hvitved added the no-change-note-required This PR does not need a change note label Aug 16, 2021
@hvitved hvitved force-pushed the files-folders-drop-columns branch from 34588d9 to 45578fa Compare August 16, 2021 10:43
geoffw0
geoffw0 previously approved these changes Aug 16, 2021
Copy link
Contributor

@geoffw0 geoffw0 left a comment

Choose a reason for hiding this comment

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

C++ changes LGTM, though note the behaviour change.

name = this.getBaseName() and
firstDotPos = min(name.indexOf(".")) and
result = name.suffix(firstDotPos + 1)
)
Copy link
Contributor

Choose a reason for hiding this comment

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

As far as I can tell this change doesn't accurately reproduce current behaviour - despite the comment, for "file.tar.gz", getExtensions() would return just "gz" and getShortName() would be "file.tar". This has probably been broken at some point, and this is a fix, but we should be aware stuff (like tests) could break.

Copy link
Contributor

Choose a reason for hiding this comment

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

Also, I think the old getExtensions() would return an empty string if there's no extension, whereas your code gives no result. This change is probably worth addressing as it seems like it could potentially break some query somewhere.

@hvitved hvitved marked this pull request as draft August 16, 2021 14:06
Copy link
Contributor

@tamasvajk tamasvajk left a comment

Choose a reason for hiding this comment

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

The C# changes LGTM. I added one minor comment/question.

Comment on lines +206 to +207
internal static Tuple files(File file, string fullName) =>
new Tuple("files", file, fullName);
Copy link
Contributor

Choose a reason for hiding this comment

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

Can we get rid of Semmle.Extraction.CIL.Tuples.files() and Semmle.Extraction.CIL.Tuples.folders() methods altogether? The relations are shared between CIL and C#, so we could use a single implementation in Semmle.Extraction.Tuples.

@geoffw0
Copy link
Contributor

geoffw0 commented Aug 18, 2021

The merge conflicts appear to come from #6488 - the locations of some files have changes.

Copy link
Contributor

@tausbn tausbn left a comment

Choose a reason for hiding this comment

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

A few comments. Apart from that this looks fine to me. 👍

Comment on lines 121 to +122
/** DEPRECATED: Use `getBaseName` instead. */
deprecated string getSimple() { folders(this, _, result) }
deprecated string getSimple() { result = this.getBaseName() }
Copy link
Contributor

Choose a reason for hiding this comment

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

This one can just be deleted. It's been deprecated for 2+ years.

Comment on lines +2 to +5
* This dbscheme is auto-generated by 'semmle/dbscheme_gen.py'.
* WARNING: Any modifications to this file will be lost.
* Relations can be changed by modifying master.py or
* by adding rules to dbscheme.template
Copy link
Contributor

Choose a reason for hiding this comment

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

Was this warning heeded? (That is, did you update dbscheme.template?)

@hvitved
Copy link
Contributor Author

hvitved commented Sep 16, 2021

Replaced by per-language PRs.

@hvitved hvitved closed this Sep 16, 2021
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
C# C++ depends on internal PR This PR should only be merged in sync with an internal Semmle PR Java JS no-change-note-required This PR does not need a change note Python
Projects
None yet
Development

Successfully merging this pull request may close these issues.

4 participants