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

Get name suffix from the build.yaml file #1898

Merged
merged 22 commits into from Dec 21, 2022

Conversation

trejdych
Copy link
Sponsor Contributor

@trejdych trejdych commented Nov 13, 2022

Hello, I'm not sure if it's the correct approach. I'd be happy to hear the feedback.

Closes #1851

@codecov
Copy link

codecov bot commented Nov 13, 2022

Codecov Report

Merging #1898 (ba117a3) into master (f6e6e4d) will not change coverage.
The diff coverage is n/a.

Additional details and impacted files

Impacted file tree graph

@@            Coverage Diff            @@
##            master     #1898   +/-   ##
=========================================
  Coverage   100.00%   100.00%           
=========================================
  Files            3         3           
  Lines           66        66           
=========================================
  Hits            66        66           

@trejdych trejdych marked this pull request as ready for review November 13, 2022 11:36
@trejdych
Copy link
Sponsor Contributor Author

I've marked it as ready for review however, I haven't written the test/tests because I... don't know how to do it. Is it possible?

@rrousselGit
Copy link
Owner

One way to test it is to make a separate project with a build.yaml dedicated for testing purposes.

There are also other ways of testing this that are more technical. If you fancy doing that, I invite you to look at how json_serializable does its tests.

One thing for sure is that a unit test for decoding the options would be neat.

@rrousselGit
Copy link
Owner

It's looking pretty good though!

@trejdych
Copy link
Sponsor Contributor Author

Thank you for the feedback! Please give me a few days. I would like to take a look at the json_serializable tests

@trejdych trejdych marked this pull request as draft November 15, 2022 20:37
@qoob23
Copy link

qoob23 commented Nov 20, 2022

@trejdych @rrousselGit Hi! Appreciate your work! What do you think about also adding a prefix option?
I think that naming is a highly opinionated thing. For example, I use pattern providerOfSomething. This way by typing "provider" I'm getting a nice list of autocompletion suggestions.

Example config might look like this:

builders:
  riverpod_generator:
    options:
      provider_name_suffix: ""
      provider_name_prefix: "providerOf" # podOf, etc...

One thing to notice here is that this case will break scoping. So if an annotated function's name starts with "_" it should be moved to the beginning of the generated provider's name.
And the first letter should be also capitalized.

I can submit an update to this PR if you don't mind or open a new one:)

@trejdych
Copy link
Sponsor Contributor Author

Hello @rrousselGit Sorry for the long delay. I've had some private issues 🙈

I've added a sample project and a unit test.
I've checked source_gen_test and I'm not sure how to use it. :(
It uses _MockBuildStep under the hood which don't do anything. Riverpod depends on BuildStep and I can't inject "our" mock. Am I missing something?

@qoob23 another thing is the name case. Currently, the generator changes case to a lower camel case so for your options it would be providerOfsomeName. But I also think that prefix would be nice. It would make possible "spec_cli convention" $providerName which is nice too.

@trejdych trejdych marked this pull request as ready for review November 27, 2022 12:26
@@ -156,6 +156,21 @@ class Home extends ConsumerWidget {
}
```

## Global configuration
Copy link
Owner

Choose a reason for hiding this comment

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

I don't like this being in the root examples folder.
it probably should be in the riverpod_generator folder

Copy link
Sponsor Contributor Author

Choose a reason for hiding this comment

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

that makes more sense. I've just changed

@rrousselGit
Copy link
Owner

Don't worry about source_gen_test, we don't need that part. It's mostly for error handling, which we don't have here

LGTM if you can move the example to the generator folder

@rrousselGit
Copy link
Owner

Looks like the CI broke with your latest change

@trejdych
Copy link
Sponsor Contributor Author

I have no idea what's wrong with CI. there is a simple flutter analyze and it works well locally :/

@trejdych
Copy link
Sponsor Contributor Author

trejdych commented Dec 4, 2022

Hello, I've "fixed" the CI but there is a coverage problem... It is "covered" by the example project. Do you want me to add any more tests to increase code coverage?

BTW. I had to increase a timeout for the 'ensure_build' test.

@rrousselGit
Copy link
Owner

Thanks. I'll look into this later

@rrousselGit rrousselGit merged commit 595349b into rrousselGit:master Dec 21, 2022
@rrousselGit
Copy link
Owner

Looking good, Thanks!

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.

[riverpod_generator] ability to change "Provider" postix to something else
3 participants