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

chore: remove testOnSave vs code setting #1222

Merged
merged 1 commit into from
Apr 8, 2022
Merged

Conversation

p0mvn
Copy link
Member

@p0mvn p0mvn commented Apr 7, 2022

Closes: #XXX

Description

#1221 (comment)


For contributor use:

  • Targeted PR against correct branch (see CONTRIBUTING.md)
  • Updated relevant documentation (docs/) or specification (x/<module>/spec/)
  • Added a relevant changelog entry to the Unreleased section in CHANGELOG.md
  • Re-reviewed Files changed in the Github PR explorer

@p0mvn p0mvn requested a review from alexanderbez April 7, 2022 21:34
@codecov-commenter
Copy link

Codecov Report

Merging #1222 (820cc57) into main (eed3294) will increase coverage by 0.00%.
The diff coverage is 25.80%.

@@           Coverage Diff           @@
##             main    #1222   +/-   ##
=======================================
  Coverage   21.03%   21.03%           
=======================================
  Files         196      196           
  Lines       25349    25365   +16     
=======================================
+ Hits         5332     5336    +4     
- Misses      19054    19068   +14     
+ Partials      963      961    -2     
Impacted Files Coverage Δ
x/gamm/keeper/pool.go 64.91% <0.00%> (ø)
x/gamm/pool-models/balancer/balancer_pool.go 63.49% <ø> (-0.40%) ⬇️
x/gamm/types/pool.go 0.00% <ø> (ø)
x/gamm/keeper/pool_service.go 56.61% <23.07%> (-0.61%) ⬇️
x/gamm/keeper/invariants.go 29.41% <29.41%> (+29.41%) ⬆️

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 10b318e...820cc57. Read the comment docs.

@alexanderbez alexanderbez requested a review from ValarDragon April 7, 2022 22:07
@ValarDragon
Copy link
Member

ValarDragon commented Apr 7, 2022

Why? This is a manual config that AFAICT, most people who I've helped setup vscode really liked,

Is this too intrusive or smth?

@p0mvn
Copy link
Member Author

p0mvn commented Apr 7, 2022

Why? This is a manual config that AFAICT, most people who I've helped setup vscode really liked,

Is this too intrusive or smth?

#1221 (comment)

I noticed that it doesn't work well with auto-save

@ValarDragon
Copy link
Member

Hrmm, this is unfortunate. So the tradeoff here is picking between:

Option 1: No test on save / auto save config

Noone has to do manual overrides,, new people to the project have to enable test on save manually for better development experience

Option 2: Test on save set

  • Better development experience for new folks
  • People who want to have auto save, have to override it

Unfortunately, reading through https://code.visualstudio.com/docs/getstarted/settings, I don't see an easy way to override workspace settings :/, seems like its really meant for just linters. (The way to do it seems to me be to doing language specific, user-set setting overrides, which seems like a bit too complex to expect people to do). Given that, I'm more in favor of Option 1, though I do really think option 2 is the right thing in principle =/

What do others think?

@alexanderbez
Copy link
Contributor

alexanderbez commented Apr 8, 2022

I'm honestly indifferent. I've never had testOnSave set, so I don't know what it's like. I always run my tests manually when needed.

@p0mvn
Copy link
Member Author

p0mvn commented Apr 8, 2022

I'm used to auto-save so I'm biased towards Option 1 where testOnSave is optional. However, if you think that it's better for everyone else, I can always override it locally, it's not a problem.

I'm also not sure about how testOnSave would work in the environment where we definitely don't want to run the tests on every save. For example, in e2e tests. I think running that concurrently or restarting too often might break the setup. That might be painful for new folks working on something like e2e tests if they don't know about this setting.

@alexanderbez
Copy link
Contributor

Yeah I don't see a reason for this to be true now that I think of it.

@ValarDragon
Copy link
Member

ValarDragon commented Apr 8, 2022

SGTM! I do think its a better UX for people new to golang, but we can instruct them to update their local config for this

@ValarDragon ValarDragon merged commit 9adf6d8 into main Apr 8, 2022
@ValarDragon ValarDragon deleted the roman/testOnSave branch April 8, 2022 16:20
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
Archived in project
Development

Successfully merging this pull request may close these issues.

4 participants