swc icon indicating copy to clipboard operation
swc copied to clipboard

refactor(cli): some general refactoring in swc_cli

Open bcmyers opened this issue 3 years ago • 3 comments

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 avatar Jun 20 '22 01:06 bcmyers

CLA assistant check
All committers have signed the CLA.

CLAassistant avatar Jun 20 '22 01:06 CLAassistant

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.

kwonoj avatar Jun 20 '22 14:06 kwonoj

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.

bcmyers avatar Jun 20 '22 18:06 bcmyers