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

Refactoring #778

Merged
merged 87 commits into from
Jun 5, 2020
Merged

Refactoring #778

merged 87 commits into from
Jun 5, 2020

Conversation

PopGoesTheWza
Copy link
Collaborator

@PopGoesTheWza PopGoesTheWza commented May 20, 2020

Fixes #<issue_number_goes_here> (it's a good idea to open an issue first for discussion)

  • npm run test succeeds.
  • npm run lint succeeds.
  • Appropriate changes to README are included in PR.

@PopGoesTheWza PopGoesTheWza requested a review from grant May 20, 2020 11:27
@PopGoesTheWza
Copy link
Collaborator Author

@grant Here is a proposal for new foundations in order to refactor @google/clasp

Currently the code base has very mixed coding styles, numerous kludgy hacks as well as some architecture problems (uncatched exceptions especially on Windows, sync/async context leaking, etc.)

I can work in order to improve this, with your ascent.

@grant
Copy link
Contributor

grant commented May 20, 2020

I would use a Google tool since this is a Google project. See my comment. Ping me after the change

@PopGoesTheWza
Copy link
Collaborator Author

@grant in #473 you mentioned lerna. Have you thought it some more?

I could set it up if you wish.

Tentative package list:

  1. clasp-core (a CommonJS library)
  2. clasp-cli (one or more CLI to use the clasp-core)
  3. ts2gas?
  4. ...
  5. profit

@grant
Copy link
Contributor

grant commented May 20, 2020

@grant in #473 you mentioned lerna. Have you thought it some more?

I could set it up if you wish.

Tentative package list:

  1. clasp-core (a CommonJS library)
  2. clasp-cli (one or more CLI to use the clasp-core)
  3. ts2gas?
  4. ...
  5. profit

I really don't work on this library anymore. There is no point in using lerna, it's not worth it. Feel free to try it if you want.

@PopGoesTheWza
Copy link
Collaborator Author

I really don't work on this library anymore.

Which bring us back to the issue of this project governance sadly :(

@KaiSpencer
Copy link

I really don't work on this library anymore.

Which bring us back to the issue of this project governance sadly :(

Thats such a shame.
@PopGoesTheWza Great work with this PR, some promising work done here.

@PopGoesTheWza
Copy link
Collaborator Author

@grant This PR now only uses gts

Changes are mostly about the code (error management, floating promises, etc.) and not about features (nothing new, maybe some stability issues fixed)

Do you know why the latest commit did not trigger Travis?

Copy link
Contributor

@grant grant left a comment

Choose a reason for hiding this comment

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

Looks good. Thanks!

@grant
Copy link
Contributor

grant commented Jun 5, 2020

Not sure about Travis

@grant
Copy link
Contributor

grant commented Jun 5, 2020

Please change title

@PopGoesTheWza PopGoesTheWza changed the title [WIP] Refactoring Refactoring Jun 5, 2020
@PopGoesTheWza PopGoesTheWza merged commit 3a1948c into google:master Jun 5, 2020
@PopGoesTheWza PopGoesTheWza deleted the refactor branch June 5, 2020 17:55
@PopGoesTheWza
Copy link
Collaborator Author

Not sure about Travis

CI Requests are issued... and succeed... but they are delayed/queued. So it is likely just a cosmetic thing.

@PopGoesTheWza
Copy link
Collaborator Author

@grant can you ask for a release? Still no one to take your place yet, right?

@grant
Copy link
Contributor

grant commented Jun 5, 2020

Sorry, I can't release. Only G Suite folks.
You may add instructions on installing clasp from source if you like.

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

Successfully merging this pull request may close these issues.

5 participants