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

Class and Namespace Name Collision #30

Closed
lawrence-laz opened this issue Dec 8, 2020 · 5 comments
Closed

Class and Namespace Name Collision #30

lawrence-laz opened this issue Dec 8, 2020 · 5 comments

Comments

@lawrence-laz
Copy link

Main class and namespace in which it is declared share the same name.

namespace Spectrogram
{
    public class Spectrogram
    {
...

This causes some inconvenience when creating an instance.

using Spectrogram;

var spec = new Spectrogram(...);
// CS0118: 'Spectrogram' is a namespace but is used like a type

I know this is a breaking change, but have you considered changing the namespace's name?

@swharden
Copy link
Owner

swharden commented Dec 23, 2020

Hi @lawrence-laz, thanks for mentioning this! I hadn't considered this problem before. I never add the using to my code and seeing Spectrogram.Spectrogram() never bothered me, but now that you pointed it out it's all I see! I wish I could go back in a time machine and name the namespace (and NuGet package) something different.

Problem and Immediate Solution

// Causes error CS0118: 'Spectrogram' is a namespace but is used like a type
using Spectrogram;
var spec = new Spectrogram(...);
// This works, but is long and looks redundant
var spec = new Spectrogram.Spectrogram(...);
// This works, but is nontraditional
using SG = Spectrogram;
var spec = new SG.Spectrogram(...);

Potential Fix: Rename the Namespace

have you considered changing the namespace's name?

I could rename the Spectrogram namespace, but this would be a very hard break for any existing code. The NuGet package is named Spectrogram too, so I think the namespace should probably stay as it is.

Potential Fix: Rename the Class

The Spectrogram class could be renamed though. The old one could be left and marked obsolete so no one's existing code would break. The problem with this is that it's hard to think of a reasonable name to describe that class...

  • SGram
  • SpGram
  • Specgram
  • Spectrogram2D
  • SpectrogramImage
  • SpectrogramGenerator
  • SpectrogramProcessor

The proposed change would allow user code to look like this:

using Spectrogram;
var spec = new SGram(...);

swharden added a commit that referenced this issue Dec 23, 2020
Rename Spectrogram.Spectrogram to Spectrogram.SGram to avoid namespace collision #30. Old class was preserved but marked obsolete.
swharden added a commit that referenced this issue Dec 23, 2020
@swharden
Copy link
Owner

I created a PR (#31) where the class is renamed. I'll probably let this sit here for a few days in case anyone wants to say anything about it before I merge it in and release a new package on NuGet. Using the old Spectrogram class should still work, but it will cause an obsolete warning. Hopefully it won't ruffle too many feathers 🤞

[Obsolete("The Spectrogram class has been renamed to SGram")]
public class Spectrogram : SGram
{
public Spectrogram
(int sampleRate, int fftSize, int stepSize, double minFreq = 0, double maxFreq = double.PositiveInfinity, int? fixedWidth = null, int offsetHz = 0) :
base(sampleRate, fftSize, stepSize, minFreq, maxFreq, fixedWidth, offsetHz)
{ }
}

@lawrence-laz
Copy link
Author

lawrence-laz commented Dec 23, 2020

Renaming a class seems like a very good alternative. 👍

The name SGram however strikes as a bit odd and cryptic. It's unusual for me to see shortened names in c#, but I guess this is subjective. To me the name SpectrogramGenerator seemed a bit better and self-descriptive, given that this class takes some inputs and then generates images, which are the actual spectrograms.

But as long as there are no collisions I am happy! And thanks for a great library 🙂

@swharden
Copy link
Owner

name SGram however strikes as a bit odd and cryptic

name SpectrogramGenerator seemed a bit better

I think I agree with you here!

// this is shorter, but cryptic
var sg = new SGram(sampleRate, fftSize: 4096, stepSize: 500, maxFreq: 3000);
sg.Add(myData);
sg.SaveImage("data.png");
// this is more to type, but more expressive
var sg = new SpectrogramGenerator(sampleRate, fftSize: 4096, stepSize: 500, maxFreq: 3000);
sg.Add(myData);
sg.SaveImage("data.png");

Expressiveness and readability should be favored over character count...

I'll sleep on this one for a day or two and come back with fresh eyes before I make a final call! Thanks again for pointing it out @lawrence-laz. I'll leave this issue open until I fix it and the new package is in NuGet

@swharden
Copy link
Owner

swharden commented Jan 4, 2021

Thanks again for raising this issue @lawrence-laz! I ended-up changing it to SpectrogramGenerator, updated all the documentation, and released version 1.3.0 on NuGet 👍

@swharden swharden closed this as completed Jan 4, 2021
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

No branches or pull requests

2 participants