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

refactor(cli): some general refactoring in swc_cli #5003

Draft
wants to merge 6 commits into
base: main
Choose a base branch
from

Conversation

bcmyers
Copy link

@bcmyers bcmyers commented Jun 20, 2022

Description:

Hi all. First time contributor here.

I saw @kwonoj 's comment in this issue and became interested in the port of swc_cli to rust. I had a couple of free hours today and so just went through trying to familiarize myself with the existing code a bit and perform a few changes.

I mostly focused on the compile command.

I noticed a couple of small improvement opportunities if you guys like them:

  • Avoid a few unnecessary allocations in places (You can get away with keeping pointers to paths in several places where before you were allocating new PathBufs)
  • Using buffered readers and writers when trying to read/write to files (this should be marginally faster)
  • Generally adding a little more context to error messages
  • Some small nits like changing &Option<PathBuf> in function signatures to what I think is the slightly more idiomatic Option<&Path>.
  • A little cleanup of some control flow here and there

At some point, I'd like to try my hand at adding some new code fill in more of the missing pieces of the cli, but, today, I just started with some general tweaks to what's already there.

Looking forward to seeing if you're interested in any of these changes.

Best,
Brian

Related issue (if exists):

#1589

@bcmyers bcmyers changed the title Some general refactoring in swc_cli refactor(cli): some general refactoring in swc_cli Jun 20, 2022
@CLAassistant
Copy link

CLAassistant commented Jun 20, 2022

CLA assistant check
All committers have signed the CLA.

@kwonoj
Copy link
Member

kwonoj commented Jun 20, 2022

I changed this so it is now OK for the user to provide,

I'd like to request to take out these changes, for 2 reasons:

  • First, this does not belong to the main purpose of PR to refactor existing implementations
  • Secondly, there are deliberate decisions to not allow changed behavior due to confusions around base path calculation among multiple inputs, which need to be addressed in PR correctly.

@bcmyers
Copy link
Author

bcmyers commented Jun 20, 2022

I changed this so it is now OK for the user to provide,

I'd like to request to take out these changes, for 2 reasons:

* First, this does not belong to the main purpose of PR to refactor existing implementations

* Secondly, there are deliberate decisions to not allow changed behavior due to confusions around base path calculation among multiple inputs, which need to be addressed in PR correctly.

Fair enough. Change in behavior removed. Now the old behavior is retained, i.e. user receives an error if they pass a directory plus 1 or more other paths.

@kdy1 kdy1 added this to the Planned milestone Jun 25, 2022
@kdy1 kdy1 modified the milestones: Planned, v1.2.210 Jul 5, 2022
@kdy1 kdy1 marked this pull request as draft August 1, 2022 03:10
@kdy1 kdy1 removed this from the Planned milestone Aug 30, 2022
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Development

Successfully merging this pull request may close these issues.

None yet

4 participants