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: reference version file instead of fvmrc #671

Closed

Conversation

mrgnhnt96
Copy link
Contributor

When working in teams, it is ideal to define the flutter versions that we are currently using and commit them to version control. But the working local version should not be checked into version control.

The problem

Updating the .fvmrc file with the current working local version of flutter causes the file to contain modifications and could potentially be committed and pushed.

The Solution

Instead of modifying the .fvmrc file, we could have a separate file, which is not version controlled, that contains the current working version.

There is already a file that exists within the project, .fvm/version, that could be used for this functionality.

Changes

This PR introduces the change to remove the flutter/flutterSdkVersion from config_model.dart effectively removing it from the .fvmrc and .fvm/fvm_config.json files.

By removing flutterSdkVersion from the .fvmrc file, the version will be retrieved from .fvm/version instead.

Copy link

vercel bot commented Feb 28, 2024

@mrgnhnt96 is attempting to deploy a commit to the FlutterTools Team on Vercel.

A member of the Team first needs to authorize it.

Copy link

vercel bot commented Feb 28, 2024

The latest updates on your projects. Learn more about Vercel for Git ↗︎

Name Status Preview Comments Updated (UTC)
fvm ❌ Failed (Inspect) Feb 28, 2024 10:35pm

@leoafarias
Copy link
Owner

leoafarias commented Feb 28, 2024

@mrgnhnt96 It seems you are on 🔥!

Removing the versioning won't be possible, as this is a critical piece that people depend on and indicates the code's version. However, I do agree with the use case.

What do you think of the idea of doing a .fvmrc_override that is used if it exists and that can be added to .gitignore? Also, this should be fairly straightforward as we just have to do a ProjectConfig load on that file and merge during the creation of the context.

Thanks for the PRs!

@mrgnhnt96
Copy link
Contributor Author

Removing the versioning won't be possible, as this is a critical piece that people depend on and indicates the code's version. However, I do agree with the use case.

Do they still need to depend on it if .fvm/version exists?

Perhaps we approach this differently:

  • If flutter is defined within .fvmrc, it is consumed & updated
  • If flutter is not defined, .fvm/version is used instead

Then its an optional entry and wouldn't break peoples current workflow.


What do you think of the idea of doing a .fvmrc_override that is used if it exists and that can be added to .gitignore? Also, this should be fairly straightforward as we just have to do a ProjectConfig load on that file and merge during the creation of the context.

This could work, but I don't think that _override is the proper naming here. I wouldn't expect fvm to update my overrides file with the changed local flutter version, which is what this file would exist to do (in this use case).

@leoafarias
Copy link
Owner

We could have another convention like .fvmrc_local. There are a lot of use cases currently dependent on fvmrc, including integration with third-party services.

Flutter version is the reason .fvmrc exists, as it becomes the source of truth.

If the Flutter version is unnecessary, isn't it better to add .fvmrc to gitignore?

I might be missing something here. Because there are files that will change if a dev changes the sdk like the pubspeck.lock might change for example.

So the case is no 100% clear

@mrgnhnt96
Copy link
Contributor Author

mrgnhnt96 commented Feb 29, 2024

After thinking about this some more, I think that my approach to this might have been wrong. Ima close this PR for now, if in the future, I find a better argument that supports this use case, then I'll re-open it!

Thanks for the quick responses here!

@mrgnhnt96 mrgnhnt96 closed this Feb 29, 2024
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

2 participants