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

refactor: renames the CompilerFilters type #4346

Merged
merged 3 commits into from
Nov 28, 2022
Merged

Conversation

dkm
Copy link
Member

@dkm dkm commented Nov 26, 2022

The type which probably started as a real enum of possible post filtering options now also includes options used for compilers' invocations.

The type was already split, but the naming was not reflecting this in the other part of the code.

This changes tries to apply a simple renaming to the type only (correponding variables are left as 'filters').

While doing so, some typing error were discovered around the GccDump feature. A fix for this will follow in a different PR.

Signed-off-by: Marc Poulhiès dkm@kataplop.net

@dkm dkm mentioned this pull request Nov 26, 2022
@dkm dkm marked this pull request as draft November 26, 2022 14:47
@dkm dkm force-pushed the dkm/rename_compilers_filters branch 2 times, most recently from 2124eb1 to 0a15ead Compare November 26, 2022 14:51
@dkm dkm marked this pull request as ready for review November 26, 2022 14:51
@mattgodbolt
Copy link
Member

Is this ready for a look, @dkm ? :)

@dkm
Copy link
Member Author

dkm commented Nov 27, 2022

Yes it is @mattgodbolt . Sorry for the code churn, I wasn't expecting this to be so long.

Copy link
Member

@mattgodbolt mattgodbolt left a comment

Choose a reason for hiding this comment

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

LGTM! Thanks for this! A few minor comments, but I'm happy and thankful for the progress. Gradual typing!!

static/components.interfaces.ts Outdated Show resolved Hide resolved
static/event-map.ts Outdated Show resolved Hide resolved
static/panes/compiler.ts Outdated Show resolved Hide resolved
types/features/filters.interfaces.ts Show resolved Hide resolved
types/features/filters.interfaces.ts Show resolved Hide resolved
@dkm
Copy link
Member Author

dkm commented Nov 27, 2022

Looks like this needs to be rebased. Will do this tomorrow :)

The type which probably started as a real enum of possible post filtering
options now also includes options used for compilers' invocations.

The type was already split, but the naming was not reflecting this in the other
part of the code.

This changes tries to apply a simple renaming to the type only (correponding
variables are left as 'filters').

While doing so, some typing error were discovered around the GccDump feature.
A fix for this will follow in a different PR.

Signed-off-by: Marc Poulhiès <dkm@kataplop.net>
@dkm dkm force-pushed the dkm/rename_compilers_filters branch from 884a20e to c174f80 Compare November 27, 2022 22:03
@dkm dkm merged commit 2fa2bbb into main Nov 28, 2022
@dkm dkm deleted the dkm/rename_compilers_filters branch November 28, 2022 20:37
@partouf
Copy link
Contributor

partouf commented Nov 29, 2022

this is now live

mattgodbolt pushed a commit that referenced this pull request Dec 20, 2022
The type which probably started as a real enum of possible post filtering
options now also includes options used for compilers' invocations.

The type was already split, but the naming was not reflecting this in the other
part of the code.

This changes tries to apply a simple renaming to the type only (corresponding
variables are left as 'filters').

While doing so, some typing error were discovered around the GccDump feature.
A fix for this will follow in a different PR.

Signed-off-by: Marc Poulhiès <dkm@kataplop.net>
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Projects
None yet
Development

Successfully merging this pull request may close these issues.

None yet

3 participants