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/improve version checking #613

Merged
merged 18 commits into from
Aug 4, 2022

Conversation

Roms1383
Copy link
Contributor

@Roms1383 Roms1383 commented Aug 3, 2022

This PR aims at improving version checking for frb_codegen.
It follows up this discussion on former PR.

Checklist

  • An issue to be fixed by this PR is listed above.
  • New tests are added to ensure new features are working. End-to-end tests are usually in the ./frb_example/pure_dart example, more specifically, rust/src/api.rs and dart/lib/main.dart.
  • The code generator has been run, and code changes are commited. (Otherwise, CI will fail.)
  • If this PR adds/changes features, documentations (in the ./book folder) are updated.
  • CI is passing.

Remark: If the PR is submitted but I do not reply for a few days, maybe I just did not see it, so please ping me.

@Roms1383
Copy link
Contributor Author

Roms1383 commented Aug 3, 2022

It's far from being ready but I have a question @fzyzcjy : some methods return a crate::error::Error, others return anyhow::Error, which leads sometimes to being required to downcast (if an inner function return anyhow based error, but top-level function is crate based error for example), or lose informations from the source of the initial error.
Could we harmonize it to make our lives easier ?

@fzyzcjy
Copy link
Owner

fzyzcjy commented Aug 3, 2022

It's far from being ready but I have a question @fzyzcjy : some methods return a crate::error::Error, others return anyhow::Error, which leads sometimes to being required to downcast (if an inner function return anyhow based error, but top-level function is crate based error for example), or lose informations from the source of the initial error. Could we harmonize it to make our lives easier ?

Totally agree! The original (very old) version was a command line app so it used anyhow, and later we added support such that it can be used as a lib, so the thiserror is used as well. Mixing the two does not sound wise.

I guess most libraries (not applications) use thiserror? So we should stick to it as well.

But seems that thiserror does not have good support about backtraces...

@Roms1383
Copy link
Contributor Author

Roms1383 commented Aug 3, 2022

Yeah this is not an easy pick especially that I would like to avoid refactoring too much bits.
The latest commit ba24776 can provide a temporary workaround in the form of :

impl From<anyhow::Error> for Error {
    fn from(e: anyhow::Error) -> Self {
        if let Some(e) = e.downcast_ref::<Error>() {
            return e.clone();
        }
        Error::StringError(e.to_string())
    }
}

@Roms1383
Copy link
Contributor Author

Roms1383 commented Aug 4, 2022

So far this PR will ensure that a dependency e.g. ffi, ffigen :

  • has been correctly specified in pubspec.yaml (not performing any range requirement check)
  • if it does, ensure that it has been correctly pinned in pubspec.lock (performing a range requirement check on the version found there)

I have the feeling that semver_constraints crate could allow to improve the logic by checking directly in pubspec.yaml, no matter if an exact version or a range of versions is specified. But to be honest I haven't looked any further yet.

Should we go step by step and review / maybe merge this one first ? @fzyzcjy

@fzyzcjy
Copy link
Owner

fzyzcjy commented Aug 4, 2022

The logic in this PR already looks quite good to me, and I am ready to briefly review and then merge it. Good job!

In my humble opinion, I guess it may also be a great choice to have a look at some other issues (feature requests / bugs), compared with further improving this version checking in future PRs.

@Roms1383 Roms1383 marked this pull request as ready for review August 4, 2022 05:59
@Roms1383
Copy link
Contributor Author

Roms1383 commented Aug 4, 2022

In my humble opinion, I guess it may also be a great choice to have a look at some other issues (feature requests / bugs), compared with further improving this version checking in future PRs.

utterly agree ^^

Copy link
Owner

@fzyzcjy fzyzcjy left a comment

Choose a reason for hiding this comment

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

Neat code :) Only some nits and I am ready to merge it

frb_codegen/src/commands.rs Outdated Show resolved Hide resolved
frb_codegen/src/tools.rs Outdated Show resolved Hide resolved
frb_codegen/src/tools.rs Outdated Show resolved Hide resolved
@fzyzcjy fzyzcjy merged commit 45a2f11 into fzyzcjy:master Aug 4, 2022
@fzyzcjy
Copy link
Owner

fzyzcjy commented Aug 4, 2022

🎉

@github-actions github-actions bot locked as resolved and limited conversation to collaborators Aug 4, 2023
Sign up for free to subscribe to this conversation on GitHub. Already have an account? Sign in.
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

2 participants