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

feat: implement a .roseau_ignore feature #7

Open
wants to merge 6 commits into
base: main
Choose a base branch
from

Conversation

jrfaller
Copy link
Contributor

No description provided.

@gus33000
Copy link

gus33000 commented Feb 16, 2024

In git's own implementation of an ignore file, there's the possibility of defining a system wide ignore file that applies retroactively in every repository, the location for it is in ~/.config/git/ignore, it could be a nice addition to have this ability as well with roseau for common directory paths that you may want to ignore (let's say you write a lot of projects that use <root>/src/test/ for example.)

This ignore file would also need to be merged with the existing ignore file.

Thinking of gitignore as well, maybe there's a need to explicitely exclude an entire folder but select files, by using the "!" character at the start of a line, what do you think?

@jrfaller
Copy link
Contributor Author

jrfaller commented Feb 16, 2024

Thanks for the thoughts about the feature. I don't know if a global ignore is useful. For Git it is relevant for system-specific files such as .DS_Store on MacOSX you don't want to see. For an API I am not sure it's very useful. Granted there is some very common stuff like *Test* or *internal* but I feel like it should be documented on a per-repo basis, at the very least so that the users have a definition file of what the API is. However, my current proposal is kind of weak and I think we would need some more stuff to have something easily usable. For instance, in GumTree I would want to include only types belonging to a subpackage of core which are not tests. It's hard to express that with my current proposal because I would need to remove by hand all packages not belonging to core. The ! could be useful. Anyway, maybe it would be nice to start with a simple mechanism and make it grow according to the use cases of users?

@gus33000
Copy link

I agree we should definitely start with a simple ignore implementation for now. Now that you mention your use case, I also see a lot of other problems to tackle if we really want the ability to dial in precisely which packages or even changes (and I think that's more the type we wanna deal with here) to exclude or not. Not to mention, the fusion of a global ignore and a local one wouldn't be straight forward.

For ignoring paths or specific types or breaking changes, you don't want to remove such types/files potentially so roseau can properly compute the API model (if you selectively exclude a few files you can end up with an incomplete api model missing on a few api breaking changes that wouldn't be directly linked to the files you removed afterall), but what matters for you is to not have roseau report breaking changes on these types. It's a tricky feature to do and I think it's better to see later about it.

@jrfaller
Copy link
Contributor Author

Another approach to my current implementation would be to post-process the breaking change list to filter some according to an inclusion/exclusion configuration. It would enable us to ignore for instance per-kind BC (like ignore method now abstract), a thing that would be hard to do with my implementation. I am not sure however that it would cover real use cases.

@tdegueul
Copy link
Contributor

Letting users specify a list of BCs they don't care about is an important use case supported by all the other tools (incl. Maracas). It's very easy to implement though: post-process the returned list of BCs.

As for name-based exclusion of symbols, the current proposal + annotation-based exclusion LGTM.

@jrfaller
Copy link
Contributor Author

OK that's good to know. Do you think the two mechanisms should use the same configuration file or there would be another configuration file for the BC kinds? Do you think that BC kind exclusion could be on a per-symbol basis?

@tdegueul
Copy link
Contributor

tdegueul commented Feb 16, 2024

BC kinds exclusion should be global, not per-symbol. It'd be simpler if both could be specified in the same file.

@tdegueul
Copy link
Contributor

On a side note, note that Java modules should be the primary way for maintainers to specify the scope of their APIs and encapsulate internal packages since JDK9 circa 2017.

We've never implemented support for Java modules in Maracas nor Roseau but that should be top priority now. It took a while but adoption is increasing (see some projects here).

@jrfaller
Copy link
Contributor Author

jrfaller commented Apr 5, 2024

OK j'ai mergé les changes (j'espère)!

Peut-être il faudrait la merger si cela fonctionne?

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.

3 participants