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

Add silence flag to the theme dev commands #4919

Open
wants to merge 1 commit into
base: main
Choose a base branch
from

Conversation

EvilGenius13
Copy link
Contributor

@EvilGenius13 EvilGenius13 commented Nov 25, 2024

WHY are these changes introduced?

Closes: #4714
I saw a slack post talking about request logs (Ex: • 12:57:27 Request » GET 200 /collections/all 51) are too noisy and they wish they could be disabled.
Fixes #0000

WHAT is this pull request doing?

Add a --silence flag to the theme dev command to disable request logs

Before:
image

After:
image

How to test your changes?

Pull down the branch
Build the branch
Run theme dev --silence and click around in your theme links like collections, products, etc.
You shouldn't see any request logs in the terminal.

Post-release steps

Measuring impact

How do we know this change was effective? Please choose one:

  • n/a - this doesn't need measurement, e.g. a linting rule or a bug-fix
  • Existing analytics will cater for this addition
  • PR includes analytics changes to measure impact

Checklist

  • I've considered possible cross-platform impacts (Mac, Linux, Windows)
  • [] I've considered possible documentation changes

Copy link
Contributor

github-actions bot commented Nov 25, 2024

Coverage report

St.
Category Percentage Covered / Total
🟡 Statements 75.42% 8743/11592
🟡 Branches 70.71% 4266/6033
🟡 Functions 75.1% 2289/3048
🟡 Lines
75.99% (+0.01% 🔼)
8266/10878
Show files with reduced coverage 🔻
St.
File Statements Branches Functions Lines
🟢
... / app-event-watcher.ts
93.83% (-1.23% 🔻)
86.49% (-2.7% 🔻)
90.48% 98.61%

Test suite run success

1968 tests passing in 892 suites.

Report generated by 🧪jest coverage report action from f338316

@EvilGenius13 EvilGenius13 force-pushed the allow-diasbling-logs-in-theme-dev branch 3 times, most recently from 112d876 to 8cbbac5 Compare November 25, 2024 19:21
@jamesmengo
Copy link
Contributor

To consider: granularity for sync logs ( file upload / download ). Do we need more granularity for how verbose the logs are?

@EvilGenius13 EvilGenius13 force-pushed the allow-diasbling-logs-in-theme-dev branch from 8cbbac5 to acc6b57 Compare November 29, 2024 20:39
@EvilGenius13 EvilGenius13 changed the title Add no-logs flag to the theme dev commands Add silence flag to the theme dev commands Nov 29, 2024
This commit adds a `--silence` flag to the `theme dev` command.
When this flag is passed, it will disable request logging.
@EvilGenius13 EvilGenius13 force-pushed the allow-diasbling-logs-in-theme-dev branch from acc6b57 to f338316 Compare November 29, 2024 20:47
@EvilGenius13 EvilGenius13 marked this pull request as ready for review November 29, 2024 21:00
@EvilGenius13 EvilGenius13 requested review from a team as code owners November 29, 2024 21:00
@frandiox
Copy link
Contributor

frandiox commented Dec 2, 2024

To consider: granularity for sync logs ( file upload / download ). Do we need more granularity for how verbose the logs are?

Perhaps we should go with a flag that we can extend later instead of (complete) --silence?
For example: --logs=none (defaults to --logs=all. We could later add --logs=request, --logs=request,sync, etc.

There's also --verbose from the CLI itself, we should ensure they don't contradict each other.

Thoughts?

@jamesmengo
Copy link
Contributor

@frandiox 100% on the same page that adding something like --server-logs=<OPTION> gives us more flexibility. One other important consideration that you mentioned is potential conflicts with current / future flags related to output.

My 2 🪙 :

  • Flag name: Something like --server-logs is my suggestion, since this makes it clear which logs this pertains to and is less likely to have collisions in the future.
  • I love your idea of being able to compose the configuration of logs by providing multiple values. I think this is much more flexible and puts the power in the hands of the users.

Copy link
Contributor

github-actions bot commented Jan 2, 2025

This PR seems inactive. If it's still relevant, please add a comment saying so. Otherwise, take no action.
→ If there's no activity within a week, then a bot will automatically close this.
Thanks for helping to improve Shopify's dev tooling and experience.

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.

[Feature]: Flag to disable request log output in theme dev
3 participants