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

[lang] Remove wildcard imports #488

Merged
merged 7 commits into from
Feb 9, 2022
Merged

[lang] Remove wildcard imports #488

merged 7 commits into from
Feb 9, 2022

Conversation

am357
Copy link
Contributor

@am357 am357 commented Dec 21, 2021

Issue #, if available: 484

Description of changes:

  • Replace the wildcard imports with explicit ones in lang package to make the code easier to read.
  • Also fixes some redundant import namespacing which was causing conflicts as a result
    of wildcard import.
  • Removes unused imports

By submitting this pull request, I confirm that my contribution is made under the terms of the Apache 2.0 license.

- Replace the wildcard imports with explicit ones to make the code easier to read.
- Also fixes some redundant import namespacing which was causing conflicts as a result
  of wildcard import.
- Removes unused imports
@codecov-commenter
Copy link

codecov-commenter commented Dec 21, 2021

Codecov Report

Merging #488 (a32e9fc) into main (fb79924) will increase coverage by 12.44%.
The diff coverage is 79.54%.

Impacted file tree graph

@@              Coverage Diff              @@
##               main     #488       +/-   ##
=============================================
+ Coverage     68.47%   80.91%   +12.44%     
- Complexity     1686     1687        +1     
=============================================
  Files           197      196        -1     
  Lines         24472    14305    -10167     
  Branches       4568     2944     -1624     
=============================================
- Hits          16756    11575     -5181     
+ Misses         6033     1776     -4257     
+ Partials       1683      954      -729     
Flag Coverage Δ
CLI 26.53% <ø> (ø)
EXAMPLES 74.49% <ø> (ø)
LANG 82.74% <79.54%> (+13.99%) ⬆️
PTS ∅ <ø> (∅)
TEST_SCRIPT 76.92% <ø> (ø)

Flags with carried forward coverage won't be shown. Click here to find out more.

Impacted Files Coverage Δ
lang/src/org/partiql/lang/Exceptions.kt 79.16% <0.00%> (ø)
.../org/partiql/lang/ast/AggregateCallSiteListMeta.kt 75.00% <ø> (ø)
...ang/src/org/partiql/lang/ast/AstDeserialization.kt 82.13% <ø> (ø)
lang/src/org/partiql/lang/ast/AstSerialization.kt 81.37% <ø> (ø)
lang/src/org/partiql/lang/ast/InternalMetas.kt 75.00% <ø> (ø)
lang/src/org/partiql/lang/ast/IsImplictJoinMeta.kt 100.00% <ø> (ø)
lang/src/org/partiql/lang/ast/IsIonLiteralMeta.kt 100.00% <ø> (ø)
...c/org/partiql/lang/ast/MemoizedMetaDeserializer.kt 50.00% <ø> (ø)
...ang/src/org/partiql/lang/ast/SourceLocationMeta.kt 60.00% <ø> (ø)
lang/src/org/partiql/lang/ast/ast.kt 86.24% <ø> (ø)
... and 58 more

Continue to review full report at Codecov.

Legend - Click here to learn more
Δ = absolute <relative> (impact), ø = not affected, ? = missing data
Powered by Codecov. Last update fb79924...a32e9fc. Read the comment docs.

@am357 am357 self-assigned this Dec 21, 2021
@am357 am357 linked an issue Dec 21, 2021 that may be closed by this pull request
@am357 am357 marked this pull request as ready for review December 21, 2021 21:03
@dlurton
Copy link
Member

dlurton commented Dec 21, 2021

I'm not opposed to this, however, FYI, we haven't already done this already done this already due to the multiple branches and the havoc it will likely cause with merge conflicts.

@am357
Copy link
Contributor Author

am357 commented Dec 21, 2021

I'm not opposed to this, however, FYI, we haven't already done this already done this already due to the multiple branches and the havoc it will likely cause with merge conflicts.

That's fair, however IMHO, the sooner we get to this the less headache we'll have down the track.

@am357 am357 requested a review from dlurton December 21, 2021 23:11
abhikuhikar
abhikuhikar previously approved these changes Dec 22, 2021
Copy link
Contributor

@abhikuhikar abhikuhikar left a comment

Choose a reason for hiding this comment

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

We have to do this at some point. Better sooner than later. Successful build is all we need. Ship it.

Copy link
Member

@alancai98 alancai98 left a comment

Choose a reason for hiding this comment

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

I agree with what's been said already. This is some important tech debt to clean up sooner rather than later. However, we are currently working to merge one branch with a lot of changes into main. To prevent further delays due to merge conflicts, I'd prefer to perform this work after the merge.

We have already held off on performing some other tech debt cleaning to post-merge (e.g.

I haven't looked too closely at the diffs, but it's entirely possible these changes won't result in many conflicts. We could merge this PR and revert it if we feel there are too many conflicts.

@am357
Copy link
Contributor Author

am357 commented Dec 22, 2021

Good call-outs everyone!

@alancai98 to move away from speculation realm by bringing more data: do we know what parts of codebase contribute to the upcoming merge? As this commit includes almost all files under ./lang, we'll likely have a better understanding of conflicts by knowing more about the changes incoming; if this is infeasible, I see the pushing the changes now and revert later (if needed) option being viable; with having a follow-on action item to merge this commit right after, in case we revert.

@am357 am357 requested a review from jpschorr December 22, 2021 04:53
@alancai98
Copy link
Member

Good call-outs everyone!

@alancai98 to move away from speculation realm by bringing more data: do we know what parts of codebase contribute to the upcoming merge? As this commit includes almost all files under ./lang, we'll likely have a better understanding of conflicts by knowing more about the changes incoming; if this is infeasible, I see the pushing the changes now and revert later (if needed) option being viable; with having a follow-on action item to merge this commit right after, in case we revert.

Pretty much all parts of the codebase will be touched. The major updates will affect the evaluator (lang/eval) and the parser (lang/syntax). From running the git merge into main, 222 files will be involved (89 new files, 122 modified, some others deleted and renamed) with 13 conflicts. While running the git merge into this branch remove-wildcard-imports will result in around the same number of files involved but 54 conflicts.

Based on that, imo, we should hold off on removing wildcard imports till after the merge. There may also be other new files coming in to main that will need the wildcard imports removed.

@am357
Copy link
Contributor Author

am357 commented Jan 3, 2022

Based on that, imo, we should hold off on removing wildcard imports till after the merge. There may also be other new files coming in to main that will need the wildcard imports removed.

Thanks for the update @alancai98! I'll set this pull request as blocked until we get the merge in place, then.

dlurton
dlurton previously approved these changes Feb 9, 2022
Copy link
Member

@alancai98 alancai98 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 putting the effort in removing the wildcard imports! There were certainly a lot of files missing our style standards.

I have some concerns about some of the extra whitespace added. I marked most locations as a nit since they didn't span too many lines of code and could be easily fixed when we add ktlint. However, I think the added whitespace in EvaluatingCompiler may make it hard to look at the git history/blame for some of the logic.

lang/src/org/partiql/lang/CompilerPipeline.kt Outdated Show resolved Hide resolved
lang/src/org/partiql/lang/CompilerPipeline.kt Outdated Show resolved Hide resolved
lang/src/org/partiql/lang/ast/IsImplictJoinMeta.kt Outdated Show resolved Hide resolved
lang/src/org/partiql/lang/ast/StatementToExprNode.kt Outdated Show resolved Hide resolved
lang/src/org/partiql/lang/ast/ast.kt Outdated Show resolved Hide resolved
lang/test/org/partiql/lang/syntax/SqlLexerTest.kt Outdated Show resolved Hide resolved
lang/src/org/partiql/lang/syntax/SqlParser.kt Outdated Show resolved Hide resolved
lang/src/org/partiql/lang/syntax/SqlParser.kt Outdated Show resolved Hide resolved
lang/src/org/partiql/lang/syntax/SqlParser.kt Outdated Show resolved Hide resolved
lang/src/org/partiql/lang/eval/EvaluatingCompiler.kt Outdated Show resolved Hide resolved
Copy link
Member

@alancai98 alancai98 left a comment

Choose a reason for hiding this comment

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

:shipit:

@am357 am357 merged commit e16f515 into main Feb 9, 2022
@am357 am357 deleted the remove-wildcard-imports branch February 9, 2022 21:30
am357 added a commit that referenced this pull request Feb 9, 2022
* [lang] Remove wildcard imports

- Replace the wildcard imports with explicit ones to make the code easier to read.
- Also fixes some redundant import namespacing which was causing conflicts as a result
  of wildcard import.
- Removes unused imports

* Optimize imports
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Projects
None yet
Development

Successfully merging this pull request may close these issues.

[lang] [source-code] remove wildcard imports
5 participants