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

reafactor: fix usings and namespaces #37

Merged
merged 1 commit into from
Nov 18, 2022
Merged

reafactor: fix usings and namespaces #37

merged 1 commit into from
Nov 18, 2022

Conversation

appie2go
Copy link
Contributor

Namespaces now match the folder structure. Although.. I doubt that's desirable? If so, rearrange the folder structure to reflect the desired namespace structure. Do note that you can use the internal keyword, not every class needs to be public. Also note that you can still test internal classes with a concept called friendly assemblies.

Next up:

  • Fixing the compiler warnings
  • Fixing the access levels
  • Some methods have too high a cognitive complexity, that needs fixing...

Holler!!

@basdijkstra
Copy link
Owner

Thanks once more, @appie2go! I created #35 and #36 specifically as a reminder to take a better look at namespace structure (and folder structure, as a result), I need to think a little harder about what I want it to look.

Making more classes internal has been on the list for a while now, I just haven't found the time to do so yet.

I didn't know about the 'friendly assemblies' concept yet, but I doubt I need it at the moment as all my tests are acceptance tests, using only the public API of the library right now?

Anyway, thanks a ton again, I'll have a good look at this PR soon. It might take me a couple of days, though, please don't think I don't appreciate the work, there's just other things to do, too :)

@basdijkstra basdijkstra self-assigned this Nov 18, 2022
@appie2go
Copy link
Contributor Author

Take your time. I know how these things go. I fixed these things when the baby was sleeping... It's my parttime day today. If you have questions I think you know where to find me, right? Don't hesitate to contact me.

@basdijkstra
Copy link
Owner

basdijkstra commented Nov 18, 2022

Take your time. I know how these things go. I fixed these things when the baby was sleeping... It's my parttime day today. If you have questions I think you know where to find me, right? Don't hesitate to contact me.

Could you send me an email at [email protected] so I have your email address? I don't think I need but you never know and I'm avoiding LinkedIn and other social media for a while. Cheers!

@basdijkstra
Copy link
Owner

I’m seriously considering replacing all references to RestAssuredNet and RestAssured.Net to simply RestAssured. Would save a lot of confusion and folder / namespace mismatches, and it’s pretty obvious this is a C# .NET project anyway.

Would that be hard to update in this PR, @appie2go ? I’m not asking you to do it just yet, just would like to learn about impact.

@appie2go
Copy link
Contributor Author

appie2go commented Nov 18, 2022

Hi Bas,

It's not a big deal to change a namespace on a project this size. You just need realise two things:

1.) Changing namespaces is a breaking change for the consumers of your library. So, it's a major change which results in a new major version. This is annoying for the consumers of your package, because they need to figure out how to fix their broken code when they update the package.(So, make sure it's documented properly and don't do it too often.)

2.) Namespaces should/must reflect folder structure. So, moving everything to one namespace/folder is probably going to be a maintenance issue after a while.

Here's what i'd suggest:
Think about what classes you want to expose to the client. Group them by feature. Create a folder per feature in the root of the project. Make the classes you want the client to use public and everything else internal. You can rearrange internal classes wherever you please all the time..

Read:

--- My persion opinion ---
Also try not to choose names/types that have been reserved by other frameworks. (Exception types have names similar to NUnit. That's annoying because you need to explicitly declare usings or define the namespace.) And yes. Call the darn thing RestAssured. Adding .net or RA to it doesn't add anything i.m.o. :-)
--- /My personal opinion ---

Hope this helps.

Cheers!

@basdijkstra
Copy link
Owner

This helps a lot!

I’m aware that changing namespaces would be a breaking change, and that’s exactly why I want to do it sooner rather than later, now that the userbase is still small ;)

I’m going to give this some more thinking and will definitely take your advice. Again, much appreciated! Lots to learn here (which was one of the reasons I started this project in the first place).

@basdijkstra basdijkstra merged commit 67703fc into basdijkstra:main Nov 18, 2022
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